balancergroup: Add trigger point to gracefully switch a child #5251
Conversation
| sbc.builder = builder | ||
| // Even if you get an add and it persists builder but doesn't start | ||
| // balancer, this would leave graceful switch being nil, in which we are | ||
| // correctly overwriting with the recent builder here as well to use later. | ||
| // The graceful switch balancer's presence is an invariant of whether the | ||
| // balancer group is closed or not (if closed, nil, if started, present). | ||
| if sbc.balancer != nil { | ||
| sbc.gracefulSwitch() | ||
| } |
There was a problem hiding this comment.
Why not make sbc.gracefulSwitch() take the builder as the input? Then it can update the field, and also do the nil check.
There was a problem hiding this comment.
Hmmm, is there a reason you prefer it this way? I switched it to this suggestion. I originally did it this way to keep it consistent with Add() (in regards to persisting the builder - maybe because object is being created there is why it makes sense there?).
There was a problem hiding this comment.
The reason I prefer this is that, sbc.balancer should ideally dealt with as a private field of the subBalancerWrapper. It should only be read by the subBalancerWrapper methods, not outside.
And with the way it is, it's like sbc.gracefulSwitch only does half of the job.
The code is not updated. Did you not push? Or haven't changed it yet?
There was a problem hiding this comment.
Weird, I swear I changed it but I must've lost it in a squash somewhere, sorry lol. Reimplemented, sending out now.
| want = []balancer.SubConn{ | ||
| sc, | ||
| } | ||
| if err := testutils.IsRoundRobin(want, subConnFromPicker(p2)); err != nil { |
There was a problem hiding this comment.
Is this checking that the picker only returns this one subconn? If so, don't use IsRoundRobin().
There was a problem hiding this comment.
Changed to just comparing deleted SubConn to wanted SubConn from Picker.
|
|
||
| // READY will move this balancer into current, causing it to Update Client | ||
| // Conn with new Picker for this specific type of balancer, not round robin. | ||
| mb.cc.UpdateState(balancer.State{ |
There was a problem hiding this comment.
Ideally, this should only happen after the SubConn state turns Ready.
And for this testing balancer, you can wrap pickfirst https://github.com/grpc/grpc-go/blob/master/pickfirst.go#L30
And override UpdateClientConnState to always delete the first address
There was a problem hiding this comment.
Switched to a wrapped PickFirst. I had to intercept an UpdateState() call with Idle as it calls that inline on the first successful UpdateState call for this to only trigger on SubConn being Ready. This also allowed me to verify that the Picker was not updated until Graceful Switch process was complete, as I now have a knob on that due to it being triggered by SubConnState change.
| sbc.builder = builder | ||
| // Even if you get an add and it persists builder but doesn't start | ||
| // balancer, this would leave graceful switch being nil, in which we are | ||
| // correctly overwriting with the recent builder here as well to use later. | ||
| // The graceful switch balancer's presence is an invariant of whether the | ||
| // balancer group is closed or not (if closed, nil, if started, present). | ||
| if sbc.balancer != nil { | ||
| sbc.gracefulSwitch() | ||
| } |
There was a problem hiding this comment.
Hmmm, is there a reason you prefer it this way? I switched it to this suggestion. I originally did it this way to keep it consistent with Add() (in regards to persisting the builder - maybe because object is being created there is why it makes sense there?).
| want = []balancer.SubConn{ | ||
| sc, | ||
| } | ||
| if err := testutils.IsRoundRobin(want, subConnFromPicker(p2)); err != nil { |
There was a problem hiding this comment.
Changed to just comparing deleted SubConn to wanted SubConn from Picker.
|
|
||
| // READY will move this balancer into current, causing it to Update Client | ||
| // Conn with new Picker for this specific type of balancer, not round robin. | ||
| mb.cc.UpdateState(balancer.State{ |
There was a problem hiding this comment.
Switched to a wrapped PickFirst. I had to intercept an UpdateState() call with Idle as it calls that inline on the first successful UpdateState call for this to only trigger on SubConn being Ready. This also allowed me to verify that the Picker was not updated until Graceful Switch process was complete, as I now have a knob on that due to it being triggered by SubConnState change.
zasweq
force-pushed
the
balancer-group-add-graceful-switch
branch
from
8e7e033
to
bfab353
Compare
zasweq
force-pushed
the
balancer-group-add-graceful-switch
branch
from
bfab353
to
48f844b
Compare
| @@ -165,6 +165,20 @@ func (sbc *subBalancerWrapper) stopBalancer() { | |||
| sbc.balancer = nil | |||
| } | |||
|
|
|||
| func (sbc *subBalancerWrapper) gracefulSwitch(builder balancer.Builder) { | |||
There was a problem hiding this comment.
Nit: move this above stopBalancer. I often treat methods close/stop as code boundaries.
This PR adds the functionality of being able to start the Graceful Switch process for a child of the balancer group. This is branched off #5245.
RELEASE NOTES: None