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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,7 +97,7 @@ func (sbc *subBalancerWrapper) startBalancer() { | |
if sbc.balancer == nil { | ||
sbc.balancer = gracefulswitch.NewBalancer(sbc, sbc.buildOpts) | ||
} | ||
sbc.group.logger.Infof("Creating child policy of type %v", sbc.builder.Name()) | ||
sbc.group.logger.Infof("Creating child policy of type %q for locality %q", sbc.builder.Name(), sbc.id) | ||
sbc.balancer.SwitchTo(sbc.builder) | ||
if sbc.ccState != nil { | ||
sbc.balancer.UpdateClientConnState(*sbc.ccState) | ||
|
@@ -298,17 +298,29 @@ func (bg *BalancerGroup) Start() { | |
bg.outgoingMu.Unlock() | ||
} | ||
|
||
// Add adds a balancer built by builder to the group, with given id. | ||
func (bg *BalancerGroup) Add(id string, builder balancer.Builder) { | ||
// AddWithClientConn adds a balancer with the given id to the group. The | ||
// balancer is built with a balancer builder registered with balancerName. The | ||
// given ClientConn is passed to the newly built balancer instead of the | ||
// onepassed to balancergroup.New(). | ||
// | ||
// TODO: Get rid of the existing Add() API and replace it with this. | ||
func (bg *BalancerGroup) AddWithClientConn(id, balancerName string, cc balancer.ClientConn) error { | ||
bg.logger.Infof("Adding child policy of type %q for locality %q", balancerName, id) | ||
builder := balancer.Get(balancerName) | ||
if builder == nil { | ||
return fmt.Errorf("unregistered balancer name %q", balancerName) | ||
} | ||
|
||
// Store data in static map, and then check to see if bg is started. | ||
bg.outgoingMu.Lock() | ||
defer bg.outgoingMu.Unlock() | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I thought about this, and since we are planning to change the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Reverted. |
||
// If the sub-balancer in cache was built with a different | ||
// balancer builder, don't use it, cleanup this old-balancer, | ||
// and behave as sub-balancer is not found in cache. | ||
|
@@ -326,7 +338,7 @@ func (bg *BalancerGroup) Add(id string, builder balancer.Builder) { | |
} | ||
if sbc == nil { | ||
sbc = &subBalancerWrapper{ | ||
ClientConn: bg.cc, | ||
ClientConn: cc, | ||
id: id, | ||
group: bg, | ||
builder: builder, | ||
|
@@ -343,11 +355,18 @@ func (bg *BalancerGroup) Add(id string, builder balancer.Builder) { | |
sbc.updateBalancerStateWithCachedPicker() | ||
} | ||
bg.idToBalancerConfig[id] = sbc | ||
bg.outgoingMu.Unlock() | ||
return nil | ||
} | ||
|
||
// Add adds a balancer built by builder to the group, with given id. | ||
func (bg *BalancerGroup) Add(id string, builder balancer.Builder) { | ||
bg.AddWithClientConn(id, builder.Name(), bg.cc) | ||
} | ||
|
||
// UpdateBuilder updates the builder for a current child, starting the Graceful | ||
// Switch process for that child. | ||
// | ||
// TODO: update this API to take the name of the new builder instead. | ||
func (bg *BalancerGroup) UpdateBuilder(id string, builder balancer.Builder) { | ||
bg.outgoingMu.Lock() | ||
// This does not deal with the balancer cache because this call should come | ||
|
@@ -369,6 +388,7 @@ func (bg *BalancerGroup) UpdateBuilder(id string, builder balancer.Builder) { | |
// closed after timeout. Cleanup work (closing sub-balancer and removing | ||
// subconns) will be done after timeout. | ||
func (bg *BalancerGroup) Remove(id string) { | ||
bg.logger.Infof("Removing child policy for locality %q", id) | ||
bg.outgoingMu.Lock() | ||
if sbToRemove, ok := bg.idToBalancerConfig[id]; ok { | ||
if bg.outgoingStarted { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I added it as a warning log because: |
||
} | ||
child.state = s | ||
|
||
// We start/stop the init timer of this child based on the new connectivity | ||
|
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.