grpc: add testing utilities for LB policy tests #2380
Conversation
| } | ||
|
|
||
| pub(crate) struct TestWorkScheduler { | ||
| pub tx_events: mpsc::UnboundedSender<TestEvent>, |
There was a problem hiding this comment.
nit: This should be pub(crate) since the struct itself is pub(crate). Same for other struct fields in this file.
| // A test channel controller that forwards calls to a channel. This allows | ||
| // tests to verify when a channel controller is asked to create subchannels or | ||
| // update the picker. |
There was a problem hiding this comment.
nit: Only /// comments show up in the struct docs. This seems like a doc comment, so it should use ///.
| impl Eq for TestSubchannel {} | ||
|
|
||
| pub(crate) enum TestEvent { | ||
| NewSubchannel(Address, Arc<dyn Subchannel>), |
There was a problem hiding this comment.
I see that the Subchannel trait has a method to return the Address. Is it possible to use that and avoid passing the Address separately?
| } | ||
| } | ||
|
|
||
| impl ForwardingSubchannel for TestSubchannel { |
There was a problem hiding this comment.
Not related to this PR: Should ForwardingSubchannel be a struct instead of a trait?
In the regular delegating pattern, all the structs implement a single interface. What is the intended use of the delegate() method? If we need to get the underlying internal Subchannel created by the channel, we could achieve this by adding an get_internal() method on the Subchannel trait returns an internal subchannel.
There was a problem hiding this comment.
The most important reason for going with what we have currently (i.e. a Subchannel trait that is not implementable and a ForwardingSubchannel trait that is implementable) is for us to be able to add new methods to the Subchannel trait without breaking users. And making the ForwardingSubchannel a trait allows us (and users in the future) to override certain functionality when they are wrapping subchannels. My understanding is that you cannot embed a struct like in Go and override only certain methods in Rust.
There was a problem hiding this comment.
I see. I have a follow-up question: why does Subchannel need to be a trait instead of a struct?
A struct with private fields and no public constructor can't be instantiated outside the module.
There was a problem hiding this comment.
We want LB policies in the tree to be able to wrap subchannels and override some of its functionality.
There was a problem hiding this comment.
To clarify, I was asking about the Subchannel being a trait, not the ForwardingSubchannel which can be implemented by LB policies.
There was a problem hiding this comment.
It is the Subchannel trait that is returned from methods like new_subchannel, subchannel_update, pick etc. And the implementation for it is provided by the ExternalSubchannel struct in the channel, which refers to an InternalSubchannel that contains the actual subchannel functionality. In tests, we could use a TestSubchannel which would also again implement the Subchannel trait.
The ForwardingSubchannel need not be implemented by all LB policies. It will be required only by LB policies that want to wrap subchannels. I can imagine this being a struct with a blanket implementation for the Subchannel trait. But I'm not able to imagine the Subchannel itself being a struct. Maybe we can discuss this if we have time during one of our meetings?
| load_balancing::{ | ||
| ChannelController, ExternalSubchannel, ForwardingSubchannel, LbState, Subchannel, | ||
| WorkScheduler, | ||
| }, |
There was a problem hiding this comment.
Based on Lucio's comments on Doug's and mine PRs, we should follow tonic's import style. We should keep only one level of nesting so that every use statement is in a single line. Here, it would mean creating a new line for
use crate::client::load_balancing::{ChannelController, ExternalSubchannel, ForwardingSubchannel, LbState, Subchannel, WorkScheduler};
There was a problem hiding this comment.
You can look into rustfmt toml settings I think we can enforce it there as well.
There was a problem hiding this comment.
I've manually edited the imports for now. I'll figure out the appropriate settings in a bit and maybe add it in a separate PR since it will end up modifying other files as well.
|
Can rebase this now, CI is fixed on master |
Done. I hope it doesn't completely mess with the review comments' positioning on the files. |
FWIW, doing a |
This PR adds a couple of utility types that will help with testing LB policies at their API level.
This was originally part of #2340, and has been split out here to unblock other LB policy PR reviews.