feat(grpc): add round_robin LB policy #2451
Conversation
| // | ||
| // The state is determined according to normal state aggregation rules, and | ||
| // the picker round-robins between all children in that state. | ||
| fn resolve_child_updates(&mut self, channel_controller: &mut dyn ChannelController) { |
There was a problem hiding this comment.
I don't particularly see it doing anything related to resolver? It is using resolve as an overloaded termm here? It seems closer to update_picker to me from the implementation.
|
|
||
| impl TestSubchannelList { | ||
| fn new(addresses: &Vec<Address>, channel_controller: &mut dyn ChannelController) -> Self { | ||
| let mut scl = TestSubchannelList { |
There was a problem hiding this comment.
nit: more descriptive name like subchannel_list
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a lot of boilerplate before the tests even start ~400+ LOC. Is this expected? Will dependency injection and mocking childmanager and pick first help here?
Additionally, I am not sure if it applies here. But go/tott/643 somewhat recommends leaning heavily towards not overabstracting utility functions to keep things dry.
| assert!( | ||
| picked[0] != picked[1].clone(), | ||
| "Should alternate between subchannels" | ||
| ); | ||
| assert_eq!(&picked[0], &picked[2]); | ||
| assert_eq!(&picked[1], &picked[3]); | ||
| assert!(picked.contains(&subchannels[0])); | ||
| assert!(picked.contains(&subchannels[1])); |
There was a problem hiding this comment.
I assume this is detereministic, so can we assert the entire container of pickers chosen?
| for child_idx in 0..self.children.len() { | ||
| let child = &mut self.children[child_idx]; | ||
| let mut channel_controller = WrappedController::new(channel_controller); | ||
| let _ = child.policy.resolver_update( |
There was a problem hiding this comment.
Do we need this variable?
|
Thanks for the amazing test descriptions. They were really amazing at smoothening the test reviews. |
3 participants
This change is based heavily on #2405, especially the tests.