balancergroup: add a ParseConfig API and remove the UpdateBuilder API
#7232
Conversation
| // Returns a non-nil error if the provided builder is not registered. | ||
| func (bg *BalancerGroup) UpdateBuilder(id, builder string) error { |
There was a problem hiding this comment.
You modified this to return an error, but don't check it at the call sites. Even though those used to panic, the danger is that a "normal" error will be returned from here in the future, and so not checking it is problematic.
We should either panic if the builder isn't registered (as the previous behavior) or check the error at the call sites.
There was a problem hiding this comment.
Removed the UpdateBuilder call and replaced with a ParseConfig similar to the gracefulswitch balancer as discussed offline.
| } | ||
| sbc.gracefulSwitch(builder) | ||
| bg.outgoingMu.Unlock() | ||
| sbc.gracefulSwitch(bldr) |
There was a problem hiding this comment.
This used to be called without the lock -- are you sure this is OK to call with the lock held?
There was a problem hiding this comment.
This was called with the lock held earlier as well. Just that in the if sbc == nil block, we were returning without releasing the lock. With the defer now, we should release it from code paths.
| logger *grpclog.PrefixLogger | ||
| cc balancer.ClientConn | ||
| logger *grpclog.PrefixLogger | ||
| csEvltr *balancer.ConnectivityStateEvaluator |
There was a problem hiding this comment.
Optional, nit: how about csEval instead? I find it much easier to say in my head.
easwars
changed the title
ParseConfig API and remove the UpdateBuilder API
|
@dfawley : This PR now has a totally different flavor. I was a little lazy to break it up. Let me know if you prefer that though. Thanks. |
xds/internal/balancer/clustermanager/e2e_test/clustermanager_test.go
Outdated
Show resolved
Hide resolved
|
Please sign the CLAs:
😝 |
This PR makes the following API change to the
balancergrouppackage:ParseConfigmethodUpdateBuildermethodgracefulswitchbalancerbalancergroupthat want to change the child policy for a particular child, should callParseConfigwith the new configuration, and callUpdateClientConnState. This will result in graceful switchover to the new child policy.This PR also has a bunch of cleanup in the
clustermanagerLB policy:balancer.ConnectivityStateManagerinstead of manually computing aggregate connectivity statebalancergroupAPI described aboveStateAggregatorimplementation inclustermanagerseems obsolete (like supporting a separatestartmethod and clearing picker state inclose).Lockedsuffix to methods where appropriateRELEASE NOTES: none