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
ringhash: handle config updates properly #5557
Changes from all commits
217e9ae
1814745
1a9db40
fbed058
6a45d8e
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 |
---|---|---|
|
@@ -259,29 +259,22 @@ func (b *ringhashBalancer) updateAddresses(addrs []resolver.Address) bool { | |
|
||
func (b *ringhashBalancer) UpdateClientConnState(s balancer.ClientConnState) error { | ||
b.logger.Infof("Received update from resolver, balancer config: %+v", pretty.ToJSON(s.BalancerConfig)) | ||
if b.config == nil { | ||
newConfig, ok := s.BalancerConfig.(*LBConfig) | ||
if !ok { | ||
return fmt.Errorf("unexpected balancer config with type: %T", s.BalancerConfig) | ||
} | ||
b.config = newConfig | ||
newConfig, ok := s.BalancerConfig.(*LBConfig) | ||
if !ok { | ||
return fmt.Errorf("unexpected balancer config with type: %T", s.BalancerConfig) | ||
} | ||
|
||
// Successful resolution; clear resolver error and ensure we return nil. | ||
b.resolverErr = nil | ||
if b.updateAddresses(s.ResolverState.Addresses) { | ||
// If addresses were updated, no matter whether it resulted in SubConn | ||
// creation/deletion, or just weight update, we will need to regenerate | ||
// the ring. | ||
var err error | ||
b.ring, err = newRing(b.subConns, b.config.MinRingSize, b.config.MaxRingSize) | ||
if err != nil { | ||
b.ResolverError(fmt.Errorf("ringhash failed to make a new ring: %v", err)) | ||
return balancer.ErrBadResolverState | ||
} | ||
b.regeneratePicker() | ||
b.cc.UpdateState(balancer.State{ConnectivityState: b.state, Picker: b.picker}) | ||
// If addresses were updated, whether it resulted in SubConn | ||
// creation/deletion, or just weight update, we need to regenerate the ring | ||
// and send a new picker. | ||
regenerateRing := b.updateAddresses(s.ResolverState.Addresses) | ||
|
||
// If the ring configuration has changed, we need to regenerate the ring and | ||
// send a new picker. | ||
if b.config == nil || b.config.MinRingSize != newConfig.MinRingSize || b.config.MaxRingSize != newConfig.MaxRingSize { | ||
regenerateRing = true | ||
} | ||
b.config = newConfig | ||
|
||
// If resolver state contains no addresses, return an error so ClientConn | ||
// will trigger re-resolve. Also records this as an resolver error, so when | ||
|
@@ -291,6 +284,17 @@ func (b *ringhashBalancer) UpdateClientConnState(s balancer.ClientConnState) err | |
b.ResolverError(errors.New("produced zero addresses")) | ||
return balancer.ErrBadResolverState | ||
} | ||
|
||
if regenerateRing { | ||
// Ring creation is guaranteed to not fail because we call newRing() | ||
// with a non-empty subConns map. | ||
b.ring = newRing(b.subConns, b.config.MinRingSize, b.config.MaxRingSize) | ||
b.regeneratePicker() | ||
b.cc.UpdateState(balancer.State{ConnectivityState: b.state, Picker: b.picker}) | ||
} | ||
|
||
// Successful resolution; clear resolver error and return nil. | ||
b.resolverErr = nil | ||
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. Again, I don't know if this breaks it, but this doesn't clear it if hits line 295, whereas in master it did. Although it feels appropriate here, but please triage if this breaks anything. 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. This one looks totally fine to me. |
||
return nil | ||
} | ||
|
||
|
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'm guessing "non-empty" encapsualtes nil and len(0)?
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.
That's right.