Conversation
easwars
left a comment
easwars
left a comment
There was a problem hiding this comment.
I havent looked at the tests yet.
|
|
||
| impl LbPolicyBuilder for RoundRobinBuilder { | ||
| fn build(&self, options: LbPolicyOptions) -> Box<dyn LbPolicy> { | ||
| super::GLOBAL_LB_REGISTRY.add_builder(WrappedPickFirstBuilder {}); |
There was a problem hiding this comment.
Nit: super::GLOBAL_LB_REGISTRY is used in two places. So, you might as well add a use statement for it. That way, the two places where it is used can simply be GLOBAL_LB_REGISTRY.
There was a problem hiding this comment.
Oh, actually looks like you already have a use statement for it and you use it that way right below. So, please change all usages to be of that pattern.
| /** | ||
| Struct for round_robin. | ||
| */ |
There was a problem hiding this comment.
Get rid of this comment. In general, use comments to talk about "why" something is being done instead of "what" is being done. In most cases, readers of the code will be able to figure out "what" is being done. But the "why" is the harder part. Of course, there are cases where comments that talk about the "what" are also useful (like where the code is a little obfuscated, or complicated and having a comment will make the reader's life easier).
| /// Register round robin as a LbPolicy. | ||
| pub fn reg() { | ||
| super::GLOBAL_LB_REGISTRY.add_builder(RoundRobinBuilder {}); | ||
| } |
There was a problem hiding this comment.
I think it would make sense to ensure that the call to add_builder happens only once even if this function is called multiple times. Using something like this might make sense: https://doc.rust-lang.org/std/sync/struct.Once.html
FYI: We need to do the same thing to the pick_first::reg function as well.
| super::GLOBAL_LB_REGISTRY.add_builder(RoundRobinBuilder {}); | ||
| } | ||
|
|
||
| struct WrapperPickFirstPolicy { |
There was a problem hiding this comment.
Naming nit. This one is named with a prefix of Wrapper while the builder is named with a prefix of Wrapped. Ideally, we should be consistent with the names.
| fn build(&self, options: LbPolicyOptions) -> Box<dyn LbPolicy> { | ||
| pick_first::reg(); | ||
| Box::new(WrapperPickFirstPolicy { | ||
| pick_first: GLOBAL_LB_REGISTRY.get_policy("pick_first").unwrap().build( |
There was a problem hiding this comment.
Use pick_first::POLICY_NAME instead of the raw string pick_first.
| // Remove duplicates. | ||
| let mut uniques = HashSet::new(); | ||
| addresses.retain(|e| uniques.insert(e.clone())); | ||
|
|
||
| // TODO(easwars): Implement address family interleaving as part of | ||
| // the dualstack implementation. |
There was a problem hiding this comment.
I don't think we need any of this here. Please correct me if I'm wrong.
| Err(error) => { | ||
| self.move_to_transient_failure(channel_controller); | ||
| if self.child_manager.has_updated() { | ||
| if let Some(pick_update) = self.child_manager.aggregate_states() { | ||
| channel_controller.update_picker(pick_update); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't quite follow the logic here. This code as I read is moving the channel state to TF when it receives an error from the NR (irrespective of whether it had received a previous good update or not). And subsequently it sends a picker containing the aggregated states of the child policies. Can you please explain what is happening here.
There was a problem hiding this comment.
I think I originally had Child Manager handling error from the resolver so I would call resolver_update on the child_manager here. I think it's best that Round Robin handles the logic directly instead so I changed it but I think I forgot to delete the last few lines of child manager aggregating state.
| ) { | ||
| self.child_manager | ||
| .subchannel_update(subchannel, state, channel_controller); | ||
| if self.child_manager.has_updated() { |
There was a problem hiding this comment.
I think we need a better name for this has_updated method on the child manager.
There was a problem hiding this comment.
Maybe child_has_updated or something
| // Build a new subchannel list with the most recent addresses received | ||
| // from the name resolver. This will start connecting from the first | ||
| // address in the list. |
There was a problem hiding this comment.
This comment is out of place. Please remove.
| if self.child_manager.has_updated() { | ||
| if let Some(pick_update) = self.child_manager.aggregate_states() { | ||
| channel_controller.update_picker(pick_update); | ||
| } | ||
| } |
There was a problem hiding this comment.
This code block is repeated 5 times. Maybe you can make a method out of it, and replace all those 5 instances with a single line that calls the newly added method.
2 participants
Add Round Robin logic and tests
@dfawley @easwars