ringhash: don't recreate subConns when update doesn't change address information #5431
Conversation
| // This is necessary (all the attributes need to be stripped) for the | ||
| // balancer to detect identical {address+weight} combination. | ||
| weightSum += a.Metadata.(uint32) | ||
| for _, a := range subConns.Keys() { |
There was a problem hiding this comment.
Let's store Keys() in a local variable, since we range over it again later. We can also get it on the first line and use len(keys) instead of subConns.Len().
| addrsSet[aNoAttrs] = struct{}{} | ||
| if scInfo, ok := b.subConns[aNoAttrs]; !ok { | ||
| modifiedAddr := addr | ||
| modifiedAddr.Attributes = nil |
There was a problem hiding this comment.
Why clear Attributes? That seems wrong, since Attributes will affect subchannel creation. I think we should be clearing BalancerAttributes if anything (or just leave it alone since it doesn't matter for comparisons?).
There was a problem hiding this comment.
If you see line 240, we use the original addr for subchannel creation, thereby passing all the attributes that we received. This modifiedAddr is used only to key the subConns map and here we are interested only in the address weight, which is currently stored as part of the BalancerAttributes field.
There was a problem hiding this comment.
I have changed this to not ignore Attributes.
| // setWeightAttribute returns a copy of addr in which the Attributes field is | ||
| // updated with weight. | ||
| func setWeightAttribute(addr resolver.Address, weight uint32) resolver.Address { | ||
| addr.Attributes = addr.Attributes.WithValue(weightAttributeKey{}, weight) |
There was a problem hiding this comment.
BalancerAttributes?
Hold up, are we saying we want an AddressMap that is sensitive to BalancerAttributes (or at least this one), i.e. attributes that do not impact subchannel uniqueness? Because that's not what AddressMap is, currently.
There was a problem hiding this comment.
I did not change the logic here wrt to what was being used from Attributes and from BalancerAttributes. And I sort of get the idea that the ringhash LB policy cares only about address weight when deciding whether it needs to recreate subConns.
But I also seem to get your point that it doesn't seem to be right to be ignoring Attributes while deciding on subchannel uniqueness. Let me check what the other implementations do. Thanks.
There was a problem hiding this comment.
I have changed the subConns map to work with the actual address. No modifications to either Attributes or BalancerAttributes.
|
The GH review page seems utterly broken now. It can't seem to keep track of comments even without a force push now. Sigh ... |
RinghashLB policy receives address weights inBalancerAttributes. Existing code was doing the following:Attributesfield in received addresses tonilBalancerAttributesfield into deprecatedMetadatafieldresolver.Addressas a map keyAs
BalancerAttributesfield was not being set tonil(after copying over the information to theMetadatafield), addresses with the same information were being considered as different map keys, leading to subConns being recreated when there was no need for it.This change does the following:
resolver.AddressMapto store a map fromresolver.Addressto an internal valueresolver.AddressMapignoresBalancerAttributes, but takes into accountAttributesWith this change, the
ringhashpolicy is able distinguish between addresses which change only in their weight information, and therefore recreates subConns only when absolutely required. Also, as an added benefit, we stop abusing the deprecatedMetadatafield.Fixes #5420
RELEASE NOTES: