Replace GracefulSwitch.switchTo with normal config passing #11204
Conversation
This is to replace switchTo(), to allow composing GracefulSwitchLb with other helpers like MultiChildLb. It also prevents users of GracefulSwitchLb from needing to use ServiceConfigUtil.
| delegate().handleResolvedAddresses(resolvedAddresses); | ||
| return; | ||
| } | ||
| Config config = (Config) resolvedAddresses.getLoadBalancingPolicyConfig(); |
There was a problem hiding this comment.
Should provide some javadoc here or at class level that the ResolvedAddresses must have a Config, not the config provided in the EDS resource.
| @@ -90,18 +97,51 @@ public String toString() { | |||
| private LoadBalancer pendingLb = defaultBalancer; | |||
| private ConnectivityState pendingState; | |||
| private SubchannelPicker pendingPicker; | |||
| private boolean switchToCalled; | |||
There was a problem hiding this comment.
You should update the class level javadoc to reflect that it is no longer necessary to do a switchTo before calling handleResolvedAddresses as the necessary logic is now done implicitly and switchTo just pins the delegate.
| if (switchToCalled) { | ||
| return delegate().acceptResolvedAddresses(resolvedAddresses); | ||
| } | ||
| Config config = (Config) resolvedAddresses.getLoadBalancingPolicyConfig(); |
There was a problem hiding this comment.
None of the classes that use GracefulSwitchLB call acceptResolvedAddresses. You have tests, but it seems like they should be throwing an exception on this cast if they don't get short circuited above.
| createLoadBalancingPolicyConfig(selection.getProvider(), selection.getConfig())); | ||
| } | ||
|
|
||
| public static Object createLoadBalancingPolicyConfig( |
There was a problem hiding this comment.
Would be good to have some javadoc here explaining that you need to call this to get the value for ResolvedAddressBuilder.setLoadBalancingPolicyConfig() for the ResolvedAddress passed to [accept|handle]ResolvedAddresses.
| helper0.updateBalancingState(READY, picker); | ||
| verify(mockHelper).updateBalancingState(READY, picker); | ||
|
|
||
| assertIsOk(gracefulSwitchLb.acceptResolvedAddresses(addressesBuilder() |
There was a problem hiding this comment.
It would be nice to have a brief comment describing what this group of lines is testing like on line 829.
| Object config = new Object(); | ||
| new EqualsTester() | ||
| .addEqualityGroup(createConfig(lbPolicies[0], config), createConfig(lbPolicies[0], config)) | ||
| .addEqualityGroup(createConfig(lbPolicies[1], config)) |
There was a problem hiding this comment.
Why shouldn't configs with different lb policies be non-equal?
|
|
||
| ClusterImplConfig(String cluster, @Nullable String edsServiceName, | ||
| @Nullable ServerInfo lrsServerInfo, @Nullable Long maxConcurrentRequests, | ||
| List<DropOverload> dropCategories, PolicySelection childPolicy, | ||
| List<DropOverload> dropCategories, | ||
| io.grpc.internal.ServiceConfigUtil.PolicySelection childPolicy, |
There was a problem hiding this comment.
Would be nice to restore the import for PolicySelection
| childPolicy.getProvider(), childPolicy.getConfig()), | ||
| tlsContext); |
There was a problem hiding this comment.
Could join lines and still be within 100 chars
This doesn't actually remove switchTo() as the API is currently public, although we may want to mark it deprecated. You may want to look at each commit separately, especially the first. The rest of the commits should look pretty similar to each other (IIRC). The tests added to GracefulSwitchLoadBalancerTest are copies of the existing tests with switchTo() replaced. You'll notice I made some changes to the existing tests before I copied them.
No rush on reviewing this. It has been sitting around on my machine since March.