-
Notifications
You must be signed in to change notification settings - Fork 3.9k
A75 Aggregate cluster fixes #12186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A75 Aggregate cluster fixes #12186
Conversation
This reverts commit ed5072e.
childLb.shutdown(); | ||
childLb = null; | ||
} | ||
delegate.shutdown(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will break the child, as once it is shut down it shouldn't be called further. And it won't be re-created when things become good again. If we shut this down, we need to replace delegate with a new instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I see ClusterResolverLoadBalancerPolicy also uses a non-null delegate of Graceful switch LB but doesn't reinstantiate it in its shutdown. Shouldn't we do this change there too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delegate (GracefulSwitchLoadBalancer) even if it not reinstantiated in the CdsLoadbalancer2 shutdown, if CdsLoadbalancer2 acceptResolvedAddresses is called again after shutdown, the delegate's acceptResolvedAddresses works just fine.
Also isn't the usage of delegate (GracefulSwitchLoadBalancer) similar here and ClusterResolverLoadBalancerPolicy which also doesn't reinstantiate the delegate in its shutdown?
… which now holds only a single cluster's state.
No description provided.