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
priority: release references to child policies which are removed #5682
Conversation
// | ||
// TODO: Get rid of the existing Add() API and replace it with this. |
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.
In internal
I'm probably fine with this, but be cautious about putting TODOs in godoc strings.
var sbc *subBalancerWrapper | ||
// If outgoingStarted is true, search in the cache. Otherwise, cache is | ||
// guaranteed to be empty, searching is unnecessary. | ||
if bg.outgoingStarted { | ||
if old, ok := bg.balancerCache.Remove(id); ok { | ||
sbc, _ = old.(*subBalancerWrapper) | ||
if sbc != nil && sbc.builder != builder { | ||
if sbc != nil && sbc.builder.Name() != builder.Name() { |
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 pointer comparison should be fine here, right? Especially if both were pulled from the registry?
String comparisons are slower, so it's probably better to still use the pointer, but this is pretty minor.
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 thought about this, and since we are planning to change the balancergroup
API to take the name instead of a balancer.Builder
, I thought name comparison was still OK here instead of the pointer. Do you have any scenarios in mind where things can go south if we do name comparisons until we switch the balancergroup
API?
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.
It shouldn't matter, as the balancer should always come from the registry, and if it doesn't, it should also have a unique name for the builder instance. I'm not sure if we're following that in all places where we use it, but my guess is the last item is the one least likely to be followed, which would also break this version of the code. The pointer comparison is more optimized and results in simpler code (possibly? it's shorter, anyway), so I do think it would be better to keep it how it was.
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.
Reverted.
@@ -162,6 +162,9 @@ func (b *priorityBalancer) handleChildStateUpdate(childName string, s balancer.S | |||
b.logger.Warningf("priority: child balancer not found for child %v", childName) | |||
return | |||
} | |||
if !child.started { | |||
return |
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.
Maybe at least an info log about receiving an update from a child we stopped? It's a race but could be of interest.
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.
Good point. I added it as a warning log because:
a. it is not a very common event, so won't flood the logs
b. if at all we are looking at a problem that has already happened and we have logs, there is more chance of this statement showing up as a warning log than as an info log.
// | ||
// `ignore` can be updated later by `updateIgnoreResolveNow`, and the update | ||
// will be propagated to all the old and new balancers built with this. | ||
func newIgnoreResolveNowBalancerBuilder(bb balancer.Builder, ignore bool) *ignoreResolveNowBalancerBuilder { |
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.
This is so much more direct now! ➕💯
Summary of changes:
started
balancer.Builder
and pass the former to thebalancergroup
when creating child policiesbalancer.ClientConn
passed in from the parent before passing it to the childrenResolveNow()
calls from children who are configured with theIgnoreReresolutionRequests
field setAdd
APIClose()
RELEASE NOTES: