xds: ignore balancing state update from downstream after LB shutdown #8134
Conversation
…he entire priority load balancer is shut down.
…after the entire weighted-target load balancer is shut down.
voidzcy
force-pushed
the
bugfix/ignore_balancing_state_update_after_lb_shutdown
branch
from
e6efda3
to
97515bb
Compare
…he entire cluster_manager load balancer is shut down.
| @@ -98,6 +100,7 @@ public void setUp() { | |||
| lbConfigInventory.put("childB", new Object()); | |||
| lbConfigInventory.put("childC", null); | |||
| clusterManagerLoadBalancer = new ClusterManagerLoadBalancer(helper); | |||
| reset(helper); | |||
There was a problem hiding this comment.
This looks like a code smell. The javadoc says "The only reason we added reset() method is to make it possible to work with container-injected mocks."
The purpose here seems to be Mockito.clearInvocations(helper), which should also be avoided as much as possible, unless you are unable to efficiently test your program.
There was a problem hiding this comment.
This is to throw away the interactions happened in the constructor (aka, calling the getters such as getSynchronizationContext() on Helper). We don't care those interactions, so just reset the mock memory.
There was a problem hiding this comment.
This is to throw away the interactions happened in the constructor (aka, calling the getters such as getSynchronizationContext() on Helper). We don't care those interactions, so just reset the mock memory.
There was a problem hiding this comment.
This is to throw away the interactions
Mockito.clearInvocations(helper) is more appropriate than reset().
There was a problem hiding this comment.
Sure, changed to use Mockito.clearInvocations().
There isn't much difference in this case. reset() is potential code smell if it is used in the middle of a test method as it probably means it is trying to verify too much. Similar to this answer, the usages of reset() in our case seems legit.
…rpc#8134) LoadBalancers should not propagate balancing state updates after itself being shutdown. For LB policies that maintain a group of child LB policies with each having its independent lifetime, balancing state update propagations from each child LB policy can go out of the lifetime of its parent easily, especially for cases that balancing state update is put to the back of the queue and not propagated up inline. For LBs that are simple pass-through in the middle of the LB tree structure, it isn't a big issue as its lifecycle would be the same as its child. Transitively, It would behave correctly as long as its downstream is doing in the right way. This change is a sanity cleanup for LB policies that maintain multiple child LB policies to preserve the invariant that further balancing state updates from their child policies will not get propagated.
…(v1.37 backport) (#8134) (#8156) LoadBalancers should not propagate balancing state updates after itself being shutdown. For LB policies that maintain a group of child LB policies with each having its independent lifetime, balancing state update propagations from each child LB policy can go out of the lifetime of its parent easily, especially for cases that balancing state update is put to the back of the queue and not propagated up inline. For LBs that are simple pass-through in the middle of the LB tree structure, it isn't a big issue as its lifecycle would be the same as its child. Transitively, It would behave correctly as long as its downstream is doing in the right way. This change is a sanity cleanup for LB policies that maintain multiple child LB policies to preserve the invariant that further balancing state updates from their child policies will not get propagated.
Similar correspondence of #8132 for xDS.
LoadBalancers should not propagate balancing state updates after itself being shutdown.
For LB policies that maintain a group of child LB policies with each having its independent lifetime, balancing state update propagations from each child LB policy can go out of the lifetime of its parent easily, especially for cases that balancing state update is put to the back of the queue and not propagated up inline.
For LBs that are simple pass-through in the middle of the LB tree structure, it isn't a big issue as its lifecycle would be the same as its child. Transitively, It would behave correctly as long as its downstream is doing in the right way.
This change is a sanity cleanup for LB policies that maintain multiple child LB policies to preserve the invariant that further balancing state updates from their child policies will not get propagated.