xds: Disable Priority LB child policy retention cache#12806
xds: Disable Priority LB child policy retention cache#12806shivaspeaks wants to merge 2 commits into
Conversation
| private SubchannelPicker currentPicker; | ||
| // Set to true if currently in the process of handling resolved addresses. | ||
| private boolean handlingResolvedAddresses; | ||
| private boolean isPriorityLbChildPolicyCacheEnabled = |
There was a problem hiding this comment.
I really want our flags to be static. Having them change can be quite confusing.
And it could be final as well, as it doesn't change. However, it is generally cleaner to change this variable in tests than changing system properties. So I'd favor keeping this non-final, make it package-private, and change the tests to use it.
Part of the trouble with changing the system property is you can get it wrong and not actually be testing any change of behavior (e.g., if the flag is static and not re-computed).
| verify(fooBalancer1).shutdown(); | ||
| verify(barBalancer0, never()).shutdown(); | ||
| } finally { | ||
| System.clearProperty("GRPC_EXPERIMENTAL_ENABLE_PRIORITY_LB_CHILD_POLICY_CACHE"); |
There was a problem hiding this comment.
Really, we want to restore the value. So we'd need to save the current value and either set the old value or clear the value (if it were null). jdcormie did make FlagResetRule, in case you want to avoid the try-finally. It supports restoring fields, not just system poperties.
There was a problem hiding this comment.
I'll restore the prev value in finally.
| if (enablePriorityLbChildPolicyCache) { | ||
| childLbState.deactivate(); | ||
| } else { | ||
| children.remove(priority); |
There was a problem hiding this comment.
"Just because" let's keep the order of the tearDown+remove lines the same as DeletionTask. Yes, it should work both ways, but we also gain nothing by rearranging it.
2 participants
Implements gRFC A115: https://github.com/grpc/proposal/blob/master/A115-remove-priority-lb-child-policy-cache.md