Make Ring Hash LB a petiole policy#10610
Conversation
YifeiZhuang
left a comment
YifeiZhuang
left a comment
There was a problem hiding this comment.
not finished yet, put what I have now.
| /** | ||
| * The default implementation not only shuts down the lb policy, it also removes this child | ||
| * from the map of children maintained by the petiole policy. Therefore, if you plan to | ||
| * reactivate this child later, then you must override this method in your extending policy. |
There was a problem hiding this comment.
What is the exact scenario that we need to re-activate? Is that for the subchannels in the picker that is different from the updated picker due to handling new addresses?
The address update that removes the old addresses and deactivate childLbState would immediately remove the key from the childLbState. Those deactivated childLbState would never be reused in the perspective of handling an update(acceptResolvedAddress), because the reusing is based on the childLbStates map key.
I wish we do not need this override, for code readability purpose.
There was a problem hiding this comment.
I've removed the logic other than resetting the flag and left it to subclasses to do what they want with complex deactivate/reactivate pairs. Right now, I'm using reactivate in RingHash tests and ClusterManagerLB has a delayed shutdown on deactivate.
There was a problem hiding this comment.
left it to subclasses to do what they want with complex deactivate/reactivate pairs.
I see. That sounds fine.
Could you elaborate on why not keep shutdown in the default MultiChildLB.ChildLbState.deactivate() and ringhash use the default?
If we decide to remove shutdown from the default deactivate(), then you have to update other LBs, which inherit MultiChildLB, on their ChildLBState.deactivate().
There was a problem hiding this comment.
The only class that overrode deactivate was ClusterManagerLoadBalancer which specifically wants the shutdown to be triggered by a timer rather than the removal from the address list.
There was a problem hiding this comment.
The only place that calls deactivate is in MultiChildLB's acceptResolvedAddressesInternal, so that method explicitly does a shutdown rather than expecting deactivate to do it. Note that logic moved to the new MultiChildLB.updateLbStateAndShutdownRemoved and is conditional on the protected method shutdownInAcceptResolvedAddresses. Therefore, the other children don't need to be updated.
larry-safran
force-pushed
the
ring_hash_dualstack
branch
from
4755408 to
5bda687
Compare
larry-safran
added
the
kokoro:force-run
…ildren just to immediately delete them.
…irst 2 elements, but rather takes the first ring element not in TF and uses that. Also, added some comments, fixed a couple of bugs in MultiChildLoadBalancer and replaced SubchannelView with direct usage of RingHashChildLbState.
… created and respond to some code review comments.
…es explicitly do the shutdowns after updating picker by default
…shutdown in acceptResolvedAddresses.
…state and removed child cleanup in acceptResolvedAddresses.
…child cleanup when processing acceptResolvedAddresses.
…y to start a connection in updateBalancingState when there aren't any active subchannels.
…and there are 2+ TF then report TF. 1 TF + idle/connecting report connecting.
larry-safran
force-pushed
the
ring_hash_dualstack
branch
from
5bda687 to
1dbf036
Compare
grpc-kokoro
removed
the
kokoro:force-run
This class is of questionable utility and generally not used.
|
We need to figure out what is going on with your rebase process. The rebase also caused weirdness with the commit message. |
5 participants
Implementation of A61 for Ring Hash
Major changes to behavior: