feat(grpc): add aggregate_states in child_manager #2363
Conversation
cjqzhao
force-pushed
the
aggregationchildmanager
branch
from
003473b to
ea15d3b
Compare
| ) -> Result<Box<dyn Iterator<Item = ChildUpdate<Endpoint>>>, Box<dyn Error + Send + Sync>> | ||
| { | ||
| let mut endpoint_to_child_map = HashMap::new(); | ||
| for endpoint in resolver_update.endpoints.clone().unwrap().iter() { |
There was a problem hiding this comment.
Does this work instead: for endpoint in &resolver_update.endpoints?
There was a problem hiding this comment.
I did for endpoint in resolver_update.endpoints.unwrap().iter() instead
There was a problem hiding this comment.
Oh yeah you need to unwrap because it's an Option.. & seems a little cleaner to me, but the compiled result is identical so 🤷.
| fn eq(&self, other: &Self) -> bool { | ||
| self.addresses == other.addresses | ||
| } |
There was a problem hiding this comment.
This is an incomplete comparison since it doesn't look at a field. It looks like, for now, Attributes derives PartialEq, so we can compare it. We could even just derive this for Endpoint too I think.
There was a problem hiding this comment.
I just derived it for Endpoint
|
|
||
| impl StubPolicyBuilder { | ||
| pub fn new(name: &'static str, funcs: StubPolicyFuncs) -> Self { | ||
| Self { name, funcs } |
There was a problem hiding this comment.
Maybe you don't need to register the builder for the tests that you are currently adding, but I think ideally the stub builder should be registered with the LB policy registry.
There was a problem hiding this comment.
This sounds good to me. I will change it to be registered.
| resolver_update: ResolverUpdate, | ||
| ) -> Result<Box<dyn Iterator<Item = ChildUpdate<Endpoint>>>, Box<dyn Error + Send + Sync>> | ||
| { | ||
| let mut sharded_endpoints = HashMap::new(); |
There was a problem hiding this comment.
Actually, why do you need a HashMap here if you are going to only return the values from the map? Can't you use a Vector instead? The child identifier is anyways part of the ChildUpdate struct which is the item returned by the returned iterator.
| created_subchannel: Option<Arc<dyn Subchannel>>, | ||
| } | ||
|
|
||
| // Defines the functions resolver_update and subchannel_update to test aggregate_states |
There was a problem hiding this comment.
It feels like I still see a lot of lines which seem excessively long. But I'm just going to let them go at this point.
| let funcs = self.funcs.clone(); | ||
| Box::new(StubPolicy { funcs }) |
There was a problem hiding this comment.
Nit/Optional: this could be single line now without the need for the funcs local variable.
| endpoints.push(Endpoint { | ||
| addresses: addresses[i].clone(), | ||
| addresses: addresses.clone(), |
There was a problem hiding this comment.
No clone here... Please take a pass through the whole PR and remove places where you use clone and then never use the original value after that.
There was a problem hiding this comment.
Ok will do. Will do better on this for future PRs
There was a problem hiding this comment.
I pushed another commit that gets rid of cloning of subchannels in the actual tests too. Let me know if that is necessary.
| // receives. | ||
| resolver_update: Some(move |update: ResolverUpdate, _, controller| { | ||
| assert_eq!(update.endpoints.iter().len(), 1); | ||
| let endpoint = update.endpoints.as_ref().unwrap()[0].clone(); |
There was a problem hiding this comment.
This looks way more complicated than it should need to be. Try endpoints.pop()?
There was a problem hiding this comment.
Done. Just pushed a commit fixing this
Aggregate states in child_manager. This PR includes unit tests for aggregate_states and added functionality in test_utils.
@easwars @dfawley