diff --git a/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java b/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java index 7855468ee6..70833416d5 100644 --- a/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java +++ b/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java @@ -255,13 +255,6 @@ final class CachingRlsLbClient { } } - Status acceptResolvedAddressFactory(ResolvedAddressFactory childLbResolvedAddressFactory) { - synchronized (lock) { - return refCountedChildPolicyWrapperFactory.acceptResolvedAddressFactory( - childLbResolvedAddressFactory); - } - } - /** * Convert the status to UNAVAILABLE and enhance the error message. * @param status status as provided by server diff --git a/rls/src/main/java/io/grpc/rls/LbPolicyConfiguration.java b/rls/src/main/java/io/grpc/rls/LbPolicyConfiguration.java index 226176d25f..4d6ceed923 100644 --- a/rls/src/main/java/io/grpc/rls/LbPolicyConfiguration.java +++ b/rls/src/main/java/io/grpc/rls/LbPolicyConfiguration.java @@ -31,7 +31,6 @@ import io.grpc.LoadBalancer.SubchannelPicker; import io.grpc.LoadBalancerProvider; import io.grpc.LoadBalancerRegistry; import io.grpc.NameResolver.ConfigOrError; -import io.grpc.Status; import io.grpc.internal.ObjectPool; import io.grpc.rls.ChildLoadBalancerHelper.ChildLoadBalancerHelperProvider; import io.grpc.rls.RlsProtoData.RouteLookupConfig; @@ -212,7 +211,7 @@ final class LbPolicyConfiguration { private final ChildLoadBalancerHelperProvider childLbHelperProvider; private final ChildLbStatusListener childLbStatusListener; private final ChildLoadBalancingPolicy childPolicy; - private ResolvedAddressFactory childLbResolvedAddressFactory; + private final ResolvedAddressFactory childLbResolvedAddressFactory; public RefCountedChildPolicyWrapperFactory( ChildLoadBalancingPolicy childPolicy, @@ -230,19 +229,6 @@ final class LbPolicyConfiguration { childLbHelperProvider.init(); } - Status acceptResolvedAddressFactory(ResolvedAddressFactory childLbResolvedAddressFactory) { - this.childLbResolvedAddressFactory = childLbResolvedAddressFactory; - Status status = Status.OK; - for (RefCountedChildPolicyWrapper wrapper : childPolicyMap.values()) { - Status newStatus = - wrapper.childPolicyWrapper.acceptResolvedAddressFactory(childLbResolvedAddressFactory); - if (!newStatus.isOk()) { - status = newStatus; - } - } - return status; - } - ChildPolicyWrapper createOrGet(String target) { // TODO(creamsoup) check if the target is valid or not RefCountedChildPolicyWrapper pooledChildPolicyWrapper = childPolicyMap.get(target); @@ -291,7 +277,6 @@ final class LbPolicyConfiguration { private final String target; private final ChildPolicyReportingHelper helper; private final LoadBalancer lb; - private final Object childLbConfig; private volatile SubchannelPicker picker; private ConnectivityState state; @@ -310,14 +295,14 @@ final class LbPolicyConfiguration { .parseLoadBalancingPolicyConfig( childPolicy.getEffectiveChildPolicy(target)); this.lb = lbProvider.newLoadBalancer(helper); - this.childLbConfig = lbConfig.getConfig(); helper.getChannelLogger().log( - ChannelLogLevel.DEBUG, "RLS child lb created. config: {0}", childLbConfig); + ChannelLogLevel.DEBUG, "RLS child lb created. config: {0}", lbConfig.getConfig()); helper.getSynchronizationContext().execute( new Runnable() { @Override public void run() { - if (!acceptResolvedAddressFactory(childLbResolvedAddressFactory).isOk()) { + if (!lb.acceptResolvedAddresses( + childLbResolvedAddressFactory.create(lbConfig.getConfig())).isOk()) { helper.refreshNameResolution(); } lb.requestConnection(); @@ -325,11 +310,6 @@ final class LbPolicyConfiguration { }); } - Status acceptResolvedAddressFactory(ResolvedAddressFactory childLbResolvedAddressFactory) { - helper.getSynchronizationContext().throwIfNotInThisSynchronizationContext(); - return lb.acceptResolvedAddresses(childLbResolvedAddressFactory.create(childLbConfig)); - } - String getTarget() { return target; } diff --git a/rls/src/main/java/io/grpc/rls/RlsLoadBalancer.java b/rls/src/main/java/io/grpc/rls/RlsLoadBalancer.java index 6e59e867e3..81ef8fdb31 100644 --- a/rls/src/main/java/io/grpc/rls/RlsLoadBalancer.java +++ b/rls/src/main/java/io/grpc/rls/RlsLoadBalancer.java @@ -79,9 +79,7 @@ final class RlsLoadBalancer extends LoadBalancer { // not required. this.lbPolicyConfiguration = lbPolicyConfiguration; } - return routeLookupClient.acceptResolvedAddressFactory( - new ChildLbResolvedAddressFactory( - resolvedAddresses.getAddresses(), resolvedAddresses.getAttributes())); + return Status.OK; } @Override diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index 58d1ff769f..c71e4dc255 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -823,9 +823,6 @@ final class XdsNameResolver extends NameResolver { if (shouldUpdateResult && routingConfig != null) { updateResolutionResult(xdsConfig); shouldUpdateResult = false; - } else { - // Need to update at least once - shouldUpdateResult = true; } // Make newly added clusters selectable by config selector and deleted clusters no longer // selectable. @@ -996,8 +993,7 @@ final class XdsNameResolver extends NameResolver { .put("routeLookupConfig", rlsPluginConfig.config()) .put( "childPolicy", - ImmutableList.of(ImmutableMap.of(XdsLbPolicies.CDS_POLICY_NAME, ImmutableMap.of( - "is_dynamic", true)))) + ImmutableList.of(ImmutableMap.of(XdsLbPolicies.CDS_POLICY_NAME, ImmutableMap.of()))) .put("childPolicyConfigTargetFieldName", "cluster") .buildOrThrow(); return ImmutableMap.of("rls_experimental", rlsConfig); diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java index 7015a43f6e..f5c40fa211 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java @@ -28,7 +28,6 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -450,7 +449,7 @@ public class XdsNameResolverTest { // Two new service config updates triggered: // - with load balancing config being able to select cluster1 and cluster2 // - with load balancing config being able to select cluster2 only - verify(mockListener, times(3)).onResult2(resultCaptor.capture()); + verify(mockListener, times(2)).onResult2(resultCaptor.capture()); assertServiceConfigForLoadBalancingConfig( Arrays.asList(cluster1, cluster2), (Map) resultCaptor.getAllValues().get(0).getServiceConfig().getConfig()); @@ -1071,9 +1070,7 @@ public class XdsNameResolverTest { assertCallSelectClusterResult(call1, configSelector, "another-cluster", 20.0); firstCall.deliverErrorStatus(); // completes previous call - // Two updates: one for XdsNameResolver releasing the cluster, and another for - // XdsDependencyManager updating the XdsConfig - verify(mockListener, times(3)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener, times(2)).onResult2(resolutionResultCaptor.capture()); result = resolutionResultCaptor.getValue(); assertServiceConfigForLoadBalancingConfig( Arrays.asList(cluster2, "another-cluster"), @@ -1103,7 +1100,7 @@ public class XdsNameResolverTest { ImmutableMap.of()))); // Two consecutive service config updates: one for removing clcuster1, // one for adding "another=cluster". - verify(mockListener, times(3)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener, times(2)).onResult2(resolutionResultCaptor.capture()); ResolutionResult result = resolutionResultCaptor.getValue(); assertServiceConfigForLoadBalancingConfig( Arrays.asList(cluster2, "another-cluster"), @@ -1158,7 +1155,7 @@ public class XdsNameResolverTest { cluster2, Collections.emptyList(), TimeUnit.SECONDS.toNanos(15L), null, false), ImmutableMap.of()))); - verify(mockListener, times(2)).onResult2(resolutionResultCaptor.capture()); + verifyNoMoreInteractions(mockListener); // no cluster added/deleted assertCallSelectClusterResult(call1, configSelector, "another-cluster", 15.0); } @@ -1190,13 +1187,7 @@ public class XdsNameResolverTest { null, false), ImmutableMap.of()))); testCall.deliverErrorStatus(); - verify(mockListener, times(3)).onResult2(resolutionResultCaptor.capture()); - assertServiceConfigForLoadBalancingConfig( - Arrays.asList(cluster1, cluster2), resolutionResultCaptor.getAllValues().get(1)); - assertServiceConfigForLoadBalancingConfig( - Arrays.asList(cluster1, cluster2), resolutionResultCaptor.getAllValues().get(2)); - assertServiceConfigForLoadBalancingConfig( - Arrays.asList(cluster1, cluster2), resolutionResultCaptor.getAllValues().get(3)); + verifyNoMoreInteractions(mockListener); } @SuppressWarnings("unchecked") @@ -1277,7 +1268,7 @@ public class XdsNameResolverTest { "routeLookupConfig", ImmutableMap.of("lookupService", "rls-cbt.googleapis.com"), "childPolicy", - ImmutableList.of(ImmutableMap.of("cds_experimental", ImmutableMap.of("is_dynamic", true))), + ImmutableList.of(ImmutableMap.of("cds_experimental", ImmutableMap.of())), "childPolicyConfigTargetFieldName", "cluster"); Map expectedClusterManagerLbConfig = ImmutableMap.of( @@ -1324,7 +1315,7 @@ public class XdsNameResolverTest { "routeLookupConfig", ImmutableMap.of("lookupService", "rls-cbt-2.googleapis.com"), "childPolicy", - ImmutableList.of(ImmutableMap.of("cds_experimental", ImmutableMap.of("is_dynamic", true))), + ImmutableList.of(ImmutableMap.of("cds_experimental", ImmutableMap.of())), "childPolicyConfigTargetFieldName", "cluster"); Map expectedClusterManagerLbConfig2 = ImmutableMap.of( @@ -1665,7 +1656,7 @@ public class XdsNameResolverTest { } private void assertClusterResolutionResult(CallInfo call, String expectedCluster) { - verify(mockListener, atLeast(1)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener).onResult2(resolutionResultCaptor.capture()); ResolutionResult result = resolutionResultCaptor.getValue(); InternalConfigSelector configSelector = result.getAttributes().get(InternalConfigSelector.KEY); assertCallSelectClusterResult(call, configSelector, expectedCluster, null); @@ -1753,13 +1744,6 @@ public class XdsNameResolverTest { return result.getAttributes().get(InternalConfigSelector.KEY); } - private static void assertServiceConfigForLoadBalancingConfig( - List clusters, ResolutionResult result) { - @SuppressWarnings("unchecked") - Map config = (Map) result.getServiceConfig().getConfig(); - assertServiceConfigForLoadBalancingConfig(clusters, config); - } - /** * Verifies the raw service config contains an xDS load balancing config for the given clusters. */ @@ -1863,9 +1847,7 @@ public class XdsNameResolverTest { + " \"lookupService\": \"rls.bigtable.google.com\"\n" + " },\n" + " \"childPolicy\": [\n" - + " {\"cds_experimental\": {\n" - + " \"is_dynamic\": true\n" - + " }}\n" + + " {\"cds_experimental\": {}}\n" + " ],\n" + " \"childPolicyConfigTargetFieldName\": \"cluster\"\n" + " }\n" @@ -2053,7 +2035,7 @@ public class XdsNameResolverTest { FaultAbort.forHeader(FaultConfig.FractionalPercent.perMillion(600_000)), null); xdsClient.deliverLdsUpdateWithFaultInjection(cluster1, httpFilterFaultConfig, null, null, null); - verify(mockListener, times(2)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener).onResult2(resolutionResultCaptor.capture()); result = resolutionResultCaptor.getValue(); configSelector = result.getAttributes().get(InternalConfigSelector.KEY); observer = startNewCall(TestMethodDescriptors.voidMethod(), configSelector, @@ -2069,7 +2051,7 @@ public class XdsNameResolverTest { FaultAbort.forHeader(FaultConfig.FractionalPercent.perMillion(0)), null); xdsClient.deliverLdsUpdateWithFaultInjection(cluster1, httpFilterFaultConfig, null, null, null); - verify(mockListener, times(3)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener).onResult2(resolutionResultCaptor.capture()); result = resolutionResultCaptor.getValue(); configSelector = result.getAttributes().get(InternalConfigSelector.KEY); observer = startNewCall(TestMethodDescriptors.voidMethod(), configSelector, @@ -2084,7 +2066,7 @@ public class XdsNameResolverTest { FaultConfig.FractionalPercent.perMillion(600_000)), null); xdsClient.deliverLdsUpdateWithFaultInjection(cluster1, httpFilterFaultConfig, null, null, null); - verify(mockListener, times(4)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener).onResult2(resolutionResultCaptor.capture()); result = resolutionResultCaptor.getValue(); configSelector = result.getAttributes().get(InternalConfigSelector.KEY); observer = startNewCall(TestMethodDescriptors.voidMethod(), configSelector, @@ -2102,7 +2084,7 @@ public class XdsNameResolverTest { FaultConfig.FractionalPercent.perMillion(400_000)), null); xdsClient.deliverLdsUpdateWithFaultInjection(cluster1, httpFilterFaultConfig, null, null, null); - verify(mockListener, times(5)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener).onResult2(resolutionResultCaptor.capture()); result = resolutionResultCaptor.getValue(); configSelector = result.getAttributes().get(InternalConfigSelector.KEY); observer = startNewCall(TestMethodDescriptors.voidMethod(), configSelector, @@ -2137,7 +2119,7 @@ public class XdsNameResolverTest { httpFilterFaultConfig = FaultConfig.create( FaultDelay.forHeader(FaultConfig.FractionalPercent.perMillion(600_000)), null, null); xdsClient.deliverLdsUpdateWithFaultInjection(cluster1, httpFilterFaultConfig, null, null, null); - verify(mockListener, times(2)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener).onResult2(resolutionResultCaptor.capture()); result = resolutionResultCaptor.getValue(); configSelector = result.getAttributes().get(InternalConfigSelector.KEY); observer = startNewCall(TestMethodDescriptors.voidMethod(), configSelector, @@ -2148,7 +2130,7 @@ public class XdsNameResolverTest { httpFilterFaultConfig = FaultConfig.create( FaultDelay.forHeader(FaultConfig.FractionalPercent.perMillion(0)), null, null); xdsClient.deliverLdsUpdateWithFaultInjection(cluster1, httpFilterFaultConfig, null, null, null); - verify(mockListener, times(3)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener).onResult2(resolutionResultCaptor.capture()); result = resolutionResultCaptor.getValue(); configSelector = result.getAttributes().get(InternalConfigSelector.KEY); observer = startNewCall(TestMethodDescriptors.voidMethod(), configSelector, @@ -2161,7 +2143,7 @@ public class XdsNameResolverTest { null, null); xdsClient.deliverLdsUpdateWithFaultInjection(cluster1, httpFilterFaultConfig, null, null, null); - verify(mockListener, times(4)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener).onResult2(resolutionResultCaptor.capture()); result = resolutionResultCaptor.getValue(); configSelector = result.getAttributes().get(InternalConfigSelector.KEY); observer = startNewCall(TestMethodDescriptors.voidMethod(), configSelector, @@ -2174,7 +2156,7 @@ public class XdsNameResolverTest { null, null); xdsClient.deliverLdsUpdateWithFaultInjection(cluster1, httpFilterFaultConfig, null, null, null); - verify(mockListener, times(5)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener).onResult2(resolutionResultCaptor.capture()); result = resolutionResultCaptor.getValue(); configSelector = result.getAttributes().get(InternalConfigSelector.KEY); observer = startNewCall(TestMethodDescriptors.voidMethod(), configSelector, @@ -2299,7 +2281,7 @@ public class XdsNameResolverTest { null); xdsClient.deliverLdsUpdateWithFaultInjection( cluster1, httpFilterFaultConfig, virtualHostFaultConfig, routeFaultConfig, null); - verify(mockListener, times(2)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener).onResult2(resolutionResultCaptor.capture()); result = resolutionResultCaptor.getValue(); configSelector = result.getAttributes().get(InternalConfigSelector.KEY); observer = startNewCall(TestMethodDescriptors.voidMethod(), configSelector, @@ -2316,7 +2298,7 @@ public class XdsNameResolverTest { xdsClient.deliverLdsUpdateWithFaultInjection( cluster1, httpFilterFaultConfig, virtualHostFaultConfig, routeFaultConfig, weightedClusterFaultConfig); - verify(mockListener, times(3)).onResult2(resolutionResultCaptor.capture()); + verify(mockListener).onResult2(resolutionResultCaptor.capture()); result = resolutionResultCaptor.getValue(); configSelector = result.getAttributes().get(InternalConfigSelector.KEY); observer = startNewCall(TestMethodDescriptors.voidMethod(), configSelector,