From 4b4cb0bd3be693cfd74208f53e1baecbb2b858d7 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Wed, 31 Aug 2022 08:36:50 -0700 Subject: [PATCH] api,core: Add LoadBalancer.acceptResolvedAddresses() (#9498) Introduces a new acceptResolvedAddresses() method in LoadBalancer that is to ultimately replace the existing handleResolvedAddresses(). The new method allows the LoadBalancer implementation to reject the given addresses by returning false from the method. The long deprecated handleResolvedAddressGroups is also removed. --- api/src/main/java/io/grpc/LoadBalancer.java | 49 ++++---- .../test/java/io/grpc/LoadBalancerTest.java | 54 +++++---- .../AutoConfiguredLoadBalancerFactory.java | 42 +++---- .../io/grpc/internal/ManagedChannelImpl.java | 9 +- .../io/grpc/util/ForwardingLoadBalancer.java | 15 +-- ...AutoConfiguredLoadBalancerFactoryTest.java | 108 +++++++++--------- .../ManagedChannelImplIdlenessTest.java | 3 +- .../grpc/internal/ManagedChannelImplTest.java | 46 +++----- .../ServiceConfigErrorHandlingTest.java | 49 ++++---- .../io/grpc/rls/CachingRlsLbClientTest.java | 4 +- 10 files changed, 183 insertions(+), 196 deletions(-) diff --git a/api/src/main/java/io/grpc/LoadBalancer.java b/api/src/main/java/io/grpc/LoadBalancer.java index 3cef6190087..6469e33b907 100644 --- a/api/src/main/java/io/grpc/LoadBalancer.java +++ b/api/src/main/java/io/grpc/LoadBalancer.java @@ -124,39 +124,46 @@ public abstract class LoadBalancer { * *

Implementations should not modify the given {@code servers}. * - * @param servers the resolved server addresses, never empty. - * @param attributes extra information from naming system. - * @deprecated override {@link #handleResolvedAddresses(ResolvedAddresses) instead} - * @since 1.2.0 + * @param resolvedAddresses the resolved server addresses, attributes, and config. + * @since 1.21.0 */ - @Deprecated - public void handleResolvedAddressGroups( - List servers, - @NameResolver.ResolutionResultAttr Attributes attributes) { + public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { if (recursionCount++ == 0) { - handleResolvedAddresses( - ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(attributes).build()); + // Note that the information about the addresses actually being accepted will be lost + // if you rely on this method for backward compatibility. + acceptResolvedAddresses(resolvedAddresses); } recursionCount = 0; } /** - * Handles newly resolved server groups and metadata attributes from name resolution system. - * {@code servers} contained in {@link EquivalentAddressGroup} should be considered equivalent - * but may be flattened into a single list if needed. + * Accepts newly resolved addresses from the name resolution system. The {@link + * EquivalentAddressGroup} addresses should be considered equivalent but may be flattened into a + * single list if needed. * - *

Implementations should not modify the given {@code servers}. + *

Implementations can choose to reject the given addresses by returning {@code false}. + * + *

Implementations should not modify the given {@code addresses}. * * @param resolvedAddresses the resolved server addresses, attributes, and config. - * @since 1.21.0 + * @return {@code true} if the resolved addresses were accepted. {@code false} if rejected. + * @since 1.49.0 */ - @SuppressWarnings("deprecation") - public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { - if (recursionCount++ == 0) { - handleResolvedAddressGroups( - resolvedAddresses.getAddresses(), resolvedAddresses.getAttributes()); + public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { + if (resolvedAddresses.getAddresses().isEmpty() + && !canHandleEmptyAddressListFromNameResolution()) { + handleNameResolutionError(Status.UNAVAILABLE.withDescription( + "NameResolver returned no usable address. addrs=" + resolvedAddresses.getAddresses() + + ", attrs=" + resolvedAddresses.getAttributes())); + return false; + } else { + if (recursionCount++ == 0) { + handleResolvedAddresses(resolvedAddresses); + } + recursionCount = 0; + + return true; } - recursionCount = 0; } /** diff --git a/api/src/test/java/io/grpc/LoadBalancerTest.java b/api/src/test/java/io/grpc/LoadBalancerTest.java index be3d10ba2ae..beaf3335e2c 100644 --- a/api/src/test/java/io/grpc/LoadBalancerTest.java +++ b/api/src/test/java/io/grpc/LoadBalancerTest.java @@ -235,13 +235,14 @@ public void createSubchannelArgs_toString() { @Deprecated @Test - public void handleResolvedAddressGroups_delegatesToHandleResolvedAddresses() { + public void handleResolvedAddresses_delegatesToAcceptResolvedAddresses() { final AtomicReference resultCapture = new AtomicReference<>(); LoadBalancer balancer = new LoadBalancer() { @Override - public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { + public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { resultCapture.set(resolvedAddresses); + return true; } @Override @@ -260,23 +261,22 @@ public void shutdown() { List servers = Arrays.asList( new EquivalentAddressGroup(new SocketAddress(){}), new EquivalentAddressGroup(new SocketAddress(){})); - balancer.handleResolvedAddressGroups(servers, attrs); + ResolvedAddresses addresses = ResolvedAddresses.newBuilder().setAddresses(servers) + .setAttributes(attrs).build(); + balancer.handleResolvedAddresses(addresses); assertThat(resultCapture.get()).isEqualTo( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(attrs).build()); } @Deprecated @Test - public void handleResolvedAddresses_delegatesToHandleResolvedAddressGroups() { - final AtomicReference> serversCapture = new AtomicReference<>(); - final AtomicReference attrsCapture = new AtomicReference<>(); + public void acceptResolvedAddresses_delegatesToHandleResolvedAddressGroups() { + final AtomicReference addressesCapture = new AtomicReference<>(); LoadBalancer balancer = new LoadBalancer() { @Override - public void handleResolvedAddressGroups( - List servers, Attributes attrs) { - serversCapture.set(servers); - attrsCapture.set(attrs); + public void handleResolvedAddresses(ResolvedAddresses addresses) { + addressesCapture.set(addresses); } @Override @@ -295,25 +295,23 @@ public void shutdown() { List servers = Arrays.asList( new EquivalentAddressGroup(new SocketAddress(){}), new EquivalentAddressGroup(new SocketAddress(){})); - balancer.handleResolvedAddresses( - ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(attrs).build()); - assertThat(serversCapture.get()).isEqualTo(servers); - assertThat(attrsCapture.get()).isEqualTo(attrs); + ResolvedAddresses addresses = ResolvedAddresses.newBuilder().setAddresses(servers) + .setAttributes(attrs).build(); + balancer.handleResolvedAddresses(addresses); + assertThat(addressesCapture.get().getAddresses()).isEqualTo(servers); + assertThat(addressesCapture.get().getAttributes()).isEqualTo(attrs); } @Deprecated @Test - public void handleResolvedAddresses_noInfiniteLoop() { - final List> serversCapture = new ArrayList<>(); - final List attrsCapture = new ArrayList<>(); + public void acceptResolvedAddresses_noInfiniteLoop() { + final List addressesCapture = new ArrayList<>(); LoadBalancer balancer = new LoadBalancer() { @Override - public void handleResolvedAddressGroups( - List servers, Attributes attrs) { - serversCapture.add(servers); - attrsCapture.add(attrs); - super.handleResolvedAddressGroups(servers, attrs); + public void handleResolvedAddresses(ResolvedAddresses addresses) { + addressesCapture.add(addresses); + super.handleResolvedAddresses(addresses); } @Override @@ -328,12 +326,12 @@ public void shutdown() { List servers = Arrays.asList( new EquivalentAddressGroup(new SocketAddress(){}), new EquivalentAddressGroup(new SocketAddress(){})); - balancer.handleResolvedAddresses( - ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(attrs).build()); - assertThat(serversCapture).hasSize(1); - assertThat(attrsCapture).hasSize(1); - assertThat(serversCapture.get(0)).isEqualTo(servers); - assertThat(attrsCapture.get(0)).isEqualTo(attrs); + ResolvedAddresses addresses = ResolvedAddresses.newBuilder().setAddresses(servers) + .setAttributes(attrs).build(); + balancer.handleResolvedAddresses(addresses); + assertThat(addressesCapture).hasSize(1); + assertThat(addressesCapture.get(0).getAddresses()).isEqualTo(servers); + assertThat(addressesCapture.get(0).getAttributes()).isEqualTo(attrs); } private static class NoopHelper extends LoadBalancer.Helper { diff --git a/core/src/main/java/io/grpc/internal/AutoConfiguredLoadBalancerFactory.java b/core/src/main/java/io/grpc/internal/AutoConfiguredLoadBalancerFactory.java index 01c48b9efcf..b8c9cab7459 100644 --- a/core/src/main/java/io/grpc/internal/AutoConfiguredLoadBalancerFactory.java +++ b/core/src/main/java/io/grpc/internal/AutoConfiguredLoadBalancerFactory.java @@ -20,11 +20,9 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; -import io.grpc.Attributes; import io.grpc.ChannelLogger.ChannelLogLevel; import io.grpc.ConnectivityState; import io.grpc.ConnectivityStateInfo; -import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.PickResult; @@ -67,10 +65,14 @@ private static final class NoopLoadBalancer extends LoadBalancer { @Override @Deprecated - public void handleResolvedAddressGroups(List s, Attributes a) {} + @SuppressWarnings("InlineMeSuggester") + public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { + } @Override - public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) {} + public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { + return true; + } @Override public void handleNameResolutionError(Status error) {} @@ -97,14 +99,10 @@ public final class AutoConfiguredLoadBalancer { } /** - * Returns non-OK status if resolvedAddresses is empty and delegate lb requires address ({@link - * LoadBalancer#canHandleEmptyAddressListFromNameResolution()} returns {@code false}). {@code - * AutoConfiguredLoadBalancer} doesn't expose {@code - * canHandleEmptyAddressListFromNameResolution} because it depends on the delegated LB. + * Returns non-OK status if the delegate rejects the resolvedAddresses (e.g. if it does not + * support an empty list). */ - Status tryHandleResolvedAddresses(ResolvedAddresses resolvedAddresses) { - List servers = resolvedAddresses.getAddresses(); - Attributes attributes = resolvedAddresses.getAttributes(); + boolean tryAcceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { PolicySelection policySelection = (PolicySelection) resolvedAddresses.getLoadBalancingPolicyConfig(); @@ -118,7 +116,7 @@ Status tryHandleResolvedAddresses(ResolvedAddresses resolvedAddresses) { delegate.shutdown(); delegateProvider = null; delegate = new NoopLoadBalancer(); - return Status.OK; + return true; } policySelection = new PolicySelection(defaultProvider, /* config= */ null); @@ -141,20 +139,12 @@ Status tryHandleResolvedAddresses(ResolvedAddresses resolvedAddresses) { ChannelLogLevel.DEBUG, "Load-balancing config: {0}", policySelection.config); } - LoadBalancer delegate = getDelegate(); - if (resolvedAddresses.getAddresses().isEmpty() - && !delegate.canHandleEmptyAddressListFromNameResolution()) { - return Status.UNAVAILABLE.withDescription( - "NameResolver returned no usable address. addrs=" + servers + ", attrs=" + attributes); - } else { - delegate.handleResolvedAddresses( - ResolvedAddresses.newBuilder() - .setAddresses(resolvedAddresses.getAddresses()) - .setAttributes(attributes) - .setLoadBalancingPolicyConfig(lbConfig) - .build()); - return Status.OK; - } + return getDelegate().acceptResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(resolvedAddresses.getAddresses()) + .setAttributes(resolvedAddresses.getAttributes()) + .setLoadBalancingPolicyConfig(lbConfig) + .build()); } void handleNameResolutionError(Status error) { diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index f7b82d1db17..6e80eb39255 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -1852,16 +1852,17 @@ public void run() { .set(LoadBalancer.ATTR_HEALTH_CHECKING_CONFIG, healthCheckingConfig) .build(); } + Attributes attributes = attrBuilder.build(); - Status handleResult = helper.lb.tryHandleResolvedAddresses( + boolean addressesAccepted = helper.lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) - .setAttributes(attrBuilder.build()) + .setAttributes(attributes) .setLoadBalancingPolicyConfig(effectiveServiceConfig.getLoadBalancingConfig()) .build()); - if (!handleResult.isOk()) { - handleErrorInSyncContext(handleResult.augmentDescription(resolver + " was used")); + if (!addressesAccepted) { + scheduleExponentialBackOffInSyncContext(); } } } diff --git a/core/src/main/java/io/grpc/util/ForwardingLoadBalancer.java b/core/src/main/java/io/grpc/util/ForwardingLoadBalancer.java index 628eda3b71b..516488f0d1a 100644 --- a/core/src/main/java/io/grpc/util/ForwardingLoadBalancer.java +++ b/core/src/main/java/io/grpc/util/ForwardingLoadBalancer.java @@ -17,14 +17,10 @@ package io.grpc.util; import com.google.common.base.MoreObjects; -import io.grpc.Attributes; import io.grpc.ConnectivityStateInfo; -import io.grpc.EquivalentAddressGroup; import io.grpc.ExperimentalApi; import io.grpc.LoadBalancer; -import io.grpc.NameResolver; import io.grpc.Status; -import java.util.List; @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1771") public abstract class ForwardingLoadBalancer extends LoadBalancer { @@ -34,16 +30,13 @@ public abstract class ForwardingLoadBalancer extends LoadBalancer { protected abstract LoadBalancer delegate(); @Override - @Deprecated - public void handleResolvedAddressGroups( - List servers, - @NameResolver.ResolutionResultAttr Attributes attributes) { - delegate().handleResolvedAddressGroups(servers, attributes); + public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { + delegate().handleResolvedAddresses(resolvedAddresses); } @Override - public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { - delegate().handleResolvedAddresses(resolvedAddresses); + public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { + return delegate().acceptResolvedAddresses(resolvedAddresses); } @Override diff --git a/core/src/test/java/io/grpc/internal/AutoConfiguredLoadBalancerFactoryTest.java b/core/src/test/java/io/grpc/internal/AutoConfiguredLoadBalancerFactoryTest.java index bf7c63818cf..ad886c31142 100644 --- a/core/src/test/java/io/grpc/internal/AutoConfiguredLoadBalancerFactoryTest.java +++ b/core/src/test/java/io/grpc/internal/AutoConfiguredLoadBalancerFactoryTest.java @@ -21,8 +21,8 @@ import static org.mockito.AdditionalAnswers.delegatesTo; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isA; import static org.mockito.ArgumentMatchers.same; -import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -97,9 +97,8 @@ public class AutoConfiguredLoadBalancerFactoryTest { @Before public void setUp() { - when(testLbBalancer.canHandleEmptyAddressListFromNameResolution()).thenCallRealMethod(); - assertThat(testLbBalancer.canHandleEmptyAddressListFromNameResolution()).isFalse(); - when(testLbBalancer2.canHandleEmptyAddressListFromNameResolution()).thenReturn(true); + when(testLbBalancer.acceptResolvedAddresses(isA(ResolvedAddresses.class))).thenReturn(true); + when(testLbBalancer2.acceptResolvedAddresses(isA(ResolvedAddresses.class))).thenReturn(true); defaultRegistry.register(testLbBalancerProvider); defaultRegistry.register(testLbBalancerProvider2); } @@ -171,7 +170,7 @@ public void shutdown() { } @Test - public void handleResolvedAddressGroups_keepOldBalancer() { + public void acceptResolvedAddresses_keepOldBalancer() { final List servers = Collections.singletonList(new EquivalentAddressGroup(new SocketAddress(){})); Helper helper = new TestHelper() { @@ -184,19 +183,19 @@ public Subchannel createSubchannel(CreateSubchannelArgs args) { AutoConfiguredLoadBalancer lb = lbf.newLoadBalancer(helper); LoadBalancer oldDelegate = lb.getDelegate(); - Status handleResult = lb.tryHandleResolvedAddresses( + boolean addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setAttributes(Attributes.EMPTY) .setLoadBalancingPolicyConfig(null) .build()); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + assertThat(addressesAccepted).isTrue(); assertThat(lb.getDelegate()).isSameInstanceAs(oldDelegate); } @Test - public void handleResolvedAddressGroups_shutsDownOldBalancer() throws Exception { + public void acceptResolvedAddresses_shutsDownOldBalancer() throws Exception { Map serviceConfig = parseConfig("{\"loadBalancingConfig\": [ {\"round_robin\": { } } ] }"); ConfigOrError lbConfigs = lbf.parseLoadBalancerPolicy(serviceConfig); @@ -226,13 +225,13 @@ public void shutdown() { }; lb.setDelegate(testlb); - Status handleResult = lb.tryHandleResolvedAddresses( + boolean addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setLoadBalancingPolicyConfig(lbConfigs.getConfig()) .build()); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + assertThat(addressesAccepted).isTrue(); assertThat(lb.getDelegateProvider().getClass().getName()).isEqualTo( "io.grpc.util.SecretRoundRobinLoadBalancerProvider$Provider"); assertTrue(shutdown.get()); @@ -240,7 +239,7 @@ public void shutdown() { @Test @SuppressWarnings("unchecked") - public void handleResolvedAddressGroups_propagateLbConfigToDelegate() throws Exception { + public void acceptResolvedAddresses_propagateLbConfigToDelegate() throws Exception { Map rawServiceConfig = parseConfig("{\"loadBalancingConfig\": [ {\"test_lb\": { \"setting1\": \"high\" } } ] }"); ConfigOrError lbConfigs = lbf.parseLoadBalancerPolicy(rawServiceConfig); @@ -251,20 +250,19 @@ public void handleResolvedAddressGroups_propagateLbConfigToDelegate() throws Exc Helper helper = new TestHelper(); AutoConfiguredLoadBalancer lb = lbf.newLoadBalancer(helper); - Status handleResult = lb.tryHandleResolvedAddresses( + boolean addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setLoadBalancingPolicyConfig(lbConfigs.getConfig()) .build()); verify(testLbBalancerProvider).newLoadBalancer(same(helper)); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + assertThat(addressesAccepted).isTrue(); assertThat(lb.getDelegate()).isSameInstanceAs(testLbBalancer); ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(testLbBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(testLbBalancer).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).containsExactlyElementsIn(servers).inOrder(); - verify(testLbBalancer, atLeast(0)).canHandleEmptyAddressListFromNameResolution(); ArgumentCaptor> lbConfigCaptor = ArgumentCaptor.forClass(Map.class); verify(testLbBalancerProvider).parseLoadBalancingPolicyConfig(lbConfigCaptor.capture()); assertThat(lbConfigCaptor.getValue()).containsExactly("setting1", "high"); @@ -274,7 +272,7 @@ public void handleResolvedAddressGroups_propagateLbConfigToDelegate() throws Exc parseConfig("{\"loadBalancingConfig\": [ {\"test_lb\": { \"setting1\": \"low\" } } ] }"); lbConfigs = lbf.parseLoadBalancerPolicy(rawServiceConfig); - handleResult = lb.tryHandleResolvedAddresses( + addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setLoadBalancingPolicyConfig(lbConfigs.getConfig()) @@ -282,8 +280,8 @@ public void handleResolvedAddressGroups_propagateLbConfigToDelegate() throws Exc resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(testLbBalancer, times(2)).handleResolvedAddresses(resultCaptor.capture()); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + verify(testLbBalancer, times(2)).acceptResolvedAddresses(resultCaptor.capture()); + assertThat(addressesAccepted).isTrue(); assertThat(resultCaptor.getValue().getAddresses()).containsExactlyElementsIn(servers).inOrder(); verify(testLbBalancerProvider, times(2)) .parseLoadBalancingPolicyConfig(lbConfigCaptor.capture()); @@ -294,7 +292,7 @@ public void handleResolvedAddressGroups_propagateLbConfigToDelegate() throws Exc } @Test - public void handleResolvedAddressGroups_propagateAddrsToDelegate() throws Exception { + public void acceptResolvedAddresses_propagateAddrsToDelegate() throws Exception { Map rawServiceConfig = parseConfig("{\"loadBalancingConfig\": [ {\"test_lb\": { \"setting1\": \"high\" } } ] }"); ConfigOrError lbConfigs = lbf.parseLoadBalancerPolicy(rawServiceConfig); @@ -305,56 +303,58 @@ public void handleResolvedAddressGroups_propagateAddrsToDelegate() throws Except List servers = Collections.singletonList(new EquivalentAddressGroup(new InetSocketAddress(8080){})); - Status handleResult = lb.tryHandleResolvedAddresses( + boolean addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setLoadBalancingPolicyConfig(lbConfigs.getConfig()) .build()); verify(testLbBalancerProvider).newLoadBalancer(same(helper)); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + assertThat(addressesAccepted).isTrue(); assertThat(lb.getDelegate()).isSameInstanceAs(testLbBalancer); ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(testLbBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(testLbBalancer).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).containsExactlyElementsIn(servers).inOrder(); servers = Collections.singletonList(new EquivalentAddressGroup(new InetSocketAddress(9090){})); - handleResult = lb.tryHandleResolvedAddresses( + addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setLoadBalancingPolicyConfig(lbConfigs.getConfig()) .build()); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); - verify(testLbBalancer, times(2)).handleResolvedAddresses(resultCaptor.capture()); + assertThat(addressesAccepted).isTrue(); + verify(testLbBalancer, times(2)).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).containsExactlyElementsIn(servers).inOrder(); } @Test - public void handleResolvedAddressGroups_delegateDoNotAcceptEmptyAddressList_nothing() + public void acceptResolvedAddresses_delegateDoNotAcceptEmptyAddressList_nothing() throws Exception { + + // The test LB will NOT accept the addresses we give them. + when(testLbBalancer.acceptResolvedAddresses(isA(ResolvedAddresses.class))).thenReturn(false); + Helper helper = new TestHelper(); AutoConfiguredLoadBalancer lb = lbf.newLoadBalancer(helper); Map serviceConfig = parseConfig("{\"loadBalancingConfig\": [ {\"test_lb\": { \"setting1\": \"high\" } } ] }"); ConfigOrError lbConfig = lbf.parseLoadBalancerPolicy(serviceConfig); - Status handleResult = lb.tryHandleResolvedAddresses( + boolean addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(Collections.emptyList()) .setLoadBalancingPolicyConfig(lbConfig.getConfig()) .build()); - assertThat(testLbBalancer.canHandleEmptyAddressListFromNameResolution()).isFalse(); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.UNAVAILABLE); - assertThat(handleResult.getDescription()).startsWith("NameResolver returned no usable address"); + assertThat(addressesAccepted).isFalse(); assertThat(lb.getDelegate()).isSameInstanceAs(testLbBalancer); } @Test - public void handleResolvedAddressGroups_delegateAcceptsEmptyAddressList() + public void acceptResolvedAddresses_delegateAcceptsEmptyAddressList() throws Exception { Helper helper = new TestHelper(); AutoConfiguredLoadBalancer lb = lbf.newLoadBalancer(helper); @@ -363,25 +363,24 @@ public void handleResolvedAddressGroups_delegateAcceptsEmptyAddressList() parseConfig("{\"loadBalancingConfig\": [ {\"test_lb2\": { \"setting1\": \"high\" } } ] }"); ConfigOrError lbConfigs = lbf.parseLoadBalancerPolicy(rawServiceConfig); - Status handleResult = lb.tryHandleResolvedAddresses( + boolean addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(Collections.emptyList()) .setLoadBalancingPolicyConfig(lbConfigs.getConfig()) .build()); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + assertThat(addressesAccepted).isTrue(); assertThat(lb.getDelegate()).isSameInstanceAs(testLbBalancer2); - assertThat(testLbBalancer2.canHandleEmptyAddressListFromNameResolution()).isTrue(); ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(testLbBalancer2).handleResolvedAddresses(resultCaptor.capture()); + verify(testLbBalancer2).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).isEmpty(); assertThat(resultCaptor.getValue().getLoadBalancingPolicyConfig()) .isEqualTo(nextParsedConfigOrError2.get().getConfig()); } @Test - public void handleResolvedAddressGroups_useSelectedLbPolicy() throws Exception { + public void acceptResolvedAddresses_useSelectedLbPolicy() throws Exception { Map rawServiceConfig = parseConfig("{\"loadBalancingConfig\": [{\"round_robin\": {}}]}"); ConfigOrError lbConfigs = lbf.parseLoadBalancerPolicy(rawServiceConfig); @@ -399,18 +398,18 @@ public Subchannel createSubchannel(CreateSubchannelArgs args) { } }; AutoConfiguredLoadBalancer lb = lbf.newLoadBalancer(helper); - Status handleResult = lb.tryHandleResolvedAddresses( + boolean addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setLoadBalancingPolicyConfig(lbConfigs.getConfig()) .build()); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + assertThat(addressesAccepted).isTrue(); assertThat(lb.getDelegate().getClass().getName()) .isEqualTo("io.grpc.util.RoundRobinLoadBalancer"); } @Test - public void handleResolvedAddressGroups_noLbPolicySelected_defaultToPickFirst() { + public void acceptResolvedAddresses_noLbPolicySelected_defaultToPickFirst() { final List servers = Collections.singletonList(new EquivalentAddressGroup(new SocketAddress(){})); Helper helper = new TestHelper() { @@ -421,27 +420,27 @@ public Subchannel createSubchannel(CreateSubchannelArgs args) { } }; AutoConfiguredLoadBalancer lb = lbf.newLoadBalancer(helper); - Status handleResult = lb.tryHandleResolvedAddresses( + boolean addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setLoadBalancingPolicyConfig(null) .build()); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + assertThat(addressesAccepted).isTrue(); assertThat(lb.getDelegate()).isInstanceOf(PickFirstLoadBalancer.class); } @Test - public void handleResolvedAddressGroups_noLbPolicySelected_defaultToCustomDefault() { + public void acceptResolvedAddresses_noLbPolicySelected_defaultToCustomDefault() { AutoConfiguredLoadBalancer lb = new AutoConfiguredLoadBalancerFactory("test_lb") .newLoadBalancer(new TestHelper()); List servers = Collections.singletonList(new EquivalentAddressGroup(new SocketAddress(){})); - Status handleResult = lb.tryHandleResolvedAddresses( + boolean addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setLoadBalancingPolicyConfig(null) .build()); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + assertThat(addressesAccepted).isTrue(); assertThat(lb.getDelegate()).isSameInstanceAs(testLbBalancer); } @@ -458,13 +457,13 @@ public Subchannel createSubchannel(CreateSubchannelArgs args) { AutoConfiguredLoadBalancer lb = new AutoConfiguredLoadBalancerFactory(GrpcUtil.DEFAULT_LB_POLICY).newLoadBalancer(helper); - Status handleResult = lb.tryHandleResolvedAddresses( + boolean addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setAttributes(Attributes.EMPTY) .build()); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + assertThat(addressesAccepted).isTrue(); verifyNoMoreInteractions(channelLogger); ConfigOrError testLbParsedConfig = ConfigOrError.fromConfig("foo"); @@ -472,13 +471,13 @@ public Subchannel createSubchannel(CreateSubchannelArgs args) { Map serviceConfig = parseConfig("{\"loadBalancingConfig\": [ {\"test_lb\": { } } ] }"); ConfigOrError lbConfigs = lbf.parseLoadBalancerPolicy(serviceConfig); - handleResult = lb.tryHandleResolvedAddresses( + addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setLoadBalancingPolicyConfig(lbConfigs.getConfig()) .build()); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + assertThat(addressesAccepted).isTrue(); verify(channelLogger).log( eq(ChannelLogLevel.INFO), eq("Load balancer changed from {0} to {1}"), @@ -495,12 +494,12 @@ public Subchannel createSubchannel(CreateSubchannelArgs args) { nextParsedConfigOrError.set(testLbParsedConfig); serviceConfig = parseConfig("{\"loadBalancingConfig\": [ {\"test_lb\": { } } ] }"); lbConfigs = lbf.parseLoadBalancerPolicy(serviceConfig); - handleResult = lb.tryHandleResolvedAddresses( + addressesAccepted = lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setLoadBalancingPolicyConfig(lbConfigs.getConfig()) .build()); - assertThat(handleResult.getCode()).isEqualTo(Status.Code.OK); + assertThat(addressesAccepted).isTrue(); verify(channelLogger).log( eq(ChannelLogLevel.DEBUG), eq("Load-balancing config: {0}"), @@ -643,14 +642,13 @@ protected LoadBalancer delegate() { @Override @Deprecated - public void handleResolvedAddressGroups( - List servers, Attributes attributes) { - delegate().handleResolvedAddressGroups(servers, attributes); + public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { + delegate().acceptResolvedAddresses(resolvedAddresses); } @Override - public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { - delegate().handleResolvedAddresses(resolvedAddresses); + public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { + return delegate().acceptResolvedAddresses(resolvedAddresses); } @Override diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java index 30e137cba22..90d2d2d8d12 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java @@ -154,6 +154,7 @@ public String getPolicyName() { @Before @SuppressWarnings("deprecation") // For NameResolver.Listener public void setUp() { + when(mockLoadBalancer.acceptResolvedAddresses(isA(ResolvedAddresses.class))).thenReturn(true); LoadBalancerRegistry.getDefaultRegistry().register(mockLoadBalancerProvider); when(mockNameResolver.getServiceAuthority()).thenReturn(AUTHORITY); when(mockNameResolverFactory @@ -220,7 +221,7 @@ public void newCallExitsIdleness() throws Exception { ArgumentCaptor resolvedAddressCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resolvedAddressCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resolvedAddressCaptor.capture()); assertThat(resolvedAddressCaptor.getValue().getAddresses()) .containsExactlyElementsIn(servers); } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index e293cb942ea..09be9a9718d 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -323,7 +323,7 @@ public void run() { @Before public void setUp() throws Exception { - when(mockLoadBalancer.canHandleEmptyAddressListFromNameResolution()).thenCallRealMethod(); + when(mockLoadBalancer.acceptResolvedAddresses(isA(ResolvedAddresses.class))).thenReturn(true); LoadBalancerRegistry.getDefaultRegistry().register(mockLoadBalancerProvider); expectedUri = new URI(TARGET); transports = TestUtils.captureTransports(mockTransportFactory); @@ -928,7 +928,7 @@ public void noMoreCallbackAfterLoadBalancerShutdown() { FakeNameResolverFactory.FakeNameResolver resolver = nameResolverFactory.resolvers.get(0); verify(mockLoadBalancerProvider).newLoadBalancer(any(Helper.class)); - verify(mockLoadBalancer).handleResolvedAddresses(resolvedAddressCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resolvedAddressCaptor.capture()); assertThat(resolvedAddressCaptor.getValue().getAddresses()).containsExactly(addressGroup); SubchannelStateListener stateListener1 = mock(SubchannelStateListener.class); @@ -1143,8 +1143,9 @@ public void nameResolutionFailed_delayedTransportShutdownCancelsBackoff() { } @Test - public void nameResolverReturnsEmptySubLists_becomeErrorByDefault() throws Exception { - String errorDescription = "NameResolver returned no usable address"; + public void nameResolverReturnsEmptySubLists_resolutionRetry() throws Exception { + // The mock LB is set to reject the addresses. + when(mockLoadBalancer.acceptResolvedAddresses(isA(ResolvedAddresses.class))).thenReturn(false); // Pass a FakeNameResolverFactory with an empty list and LB config FakeNameResolverFactory nameResolverFactory = @@ -1157,21 +1158,12 @@ public void nameResolverReturnsEmptySubLists_becomeErrorByDefault() throws Excep channelBuilder.nameResolverFactory(nameResolverFactory); createChannel(); - // LoadBalancer received the error - verify(mockLoadBalancerProvider).newLoadBalancer(any(Helper.class)); - verify(mockLoadBalancer).handleNameResolutionError(statusCaptor.capture()); - Status status = statusCaptor.getValue(); - assertSame(Status.Code.UNAVAILABLE, status.getCode()); - assertThat(status.getDescription()).startsWith(errorDescription); - // A resolution retry has been scheduled assertEquals(1, timer.numPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER)); } @Test public void nameResolverReturnsEmptySubLists_optionallyAllowed() throws Exception { - when(mockLoadBalancer.canHandleEmptyAddressListFromNameResolution()).thenReturn(true); - // Pass a FakeNameResolverFactory with an empty list and LB config FakeNameResolverFactory nameResolverFactory = new FakeNameResolverFactory.Builder(expectedUri).build(); @@ -1193,7 +1185,7 @@ public void nameResolverReturnsEmptySubLists_optionallyAllowed() throws Exceptio verify(mockLoadBalancerProvider).newLoadBalancer(any(Helper.class)); ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).isEmpty(); assertThat(resultCaptor.getValue().getLoadBalancingPolicyConfig()).isEqualTo(parsedLbConfig); @@ -1214,7 +1206,7 @@ public void loadBalancerThrowsInHandleResolvedAddresses() { createChannel(); verify(mockLoadBalancerProvider).newLoadBalancer(any(Helper.class)); - doThrow(ex).when(mockLoadBalancer).handleResolvedAddresses(any(ResolvedAddresses.class)); + doThrow(ex).when(mockLoadBalancer).acceptResolvedAddresses(any(ResolvedAddresses.class)); // NameResolver returns addresses. nameResolverFactory.allResolved(); @@ -1276,7 +1268,7 @@ public void firstResolvedServerFailedToConnect() throws Exception { // Simulate name resolution results EquivalentAddressGroup addressGroup = new EquivalentAddressGroup(resolvedAddrs); - inOrder.verify(mockLoadBalancer).handleResolvedAddresses(resolvedAddressCaptor.capture()); + inOrder.verify(mockLoadBalancer).acceptResolvedAddresses(resolvedAddressCaptor.capture()); assertThat(resolvedAddressCaptor.getValue().getAddresses()).containsExactly(addressGroup); Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); @@ -1426,7 +1418,7 @@ public void allServersFailedToConnect() throws Exception { // Simulate name resolution results EquivalentAddressGroup addressGroup = new EquivalentAddressGroup(resolvedAddrs); - inOrder.verify(mockLoadBalancer).handleResolvedAddresses(resolvedAddressCaptor.capture()); + inOrder.verify(mockLoadBalancer).acceptResolvedAddresses(resolvedAddressCaptor.capture()); assertThat(resolvedAddressCaptor.getValue().getAddresses()).containsExactly(addressGroup); Subchannel subchannel = @@ -3655,7 +3647,7 @@ public double nextDouble() { ArgumentCaptor helperCaptor = ArgumentCaptor.forClass(Helper.class); verify(mockLoadBalancerProvider).newLoadBalancer(helperCaptor.capture()); helper = helperCaptor.getValue(); - verify(mockLoadBalancer).handleResolvedAddresses( + verify(mockLoadBalancer).acceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(nameResolverFactory.servers) .build()); @@ -3761,7 +3753,7 @@ public void hedgingScheduledThenChannelShutdown_hedgeShouldStillHappen_newCallSh ArgumentCaptor helperCaptor = ArgumentCaptor.forClass(Helper.class); verify(mockLoadBalancerProvider).newLoadBalancer(helperCaptor.capture()); helper = helperCaptor.getValue(); - verify(mockLoadBalancer).handleResolvedAddresses( + verify(mockLoadBalancer).acceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(nameResolverFactory.servers) .build()); @@ -4088,7 +4080,7 @@ public void disableServiceConfigLookUp_noDefaultConfig() throws Exception { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).containsExactly(addressGroup); assertThat(resultCaptor.getValue().getAttributes().get(InternalConfigSelector.KEY)).isNull(); verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class)); @@ -4126,7 +4118,7 @@ public void disableServiceConfigLookUp_withDefaultConfig() throws Exception { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).containsExactly(addressGroup); assertThat(resultCaptor.getValue().getAttributes().get(InternalConfigSelector.KEY)).isNull(); verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class)); @@ -4156,7 +4148,7 @@ public void enableServiceConfigLookUp_noDefaultConfig() throws Exception { createChannel(); ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).containsExactly(addressGroup); verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class)); @@ -4172,7 +4164,7 @@ public void enableServiceConfigLookUp_noDefaultConfig() throws Exception { nameResolverFactory.allResolved(); resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer, times(2)).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer, times(2)).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).containsExactly(addressGroup); verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class)); } finally { @@ -4206,7 +4198,7 @@ public void enableServiceConfigLookUp_withDefaultConfig() throws Exception { createChannel(); ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).containsExactly(addressGroup); verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class)); } finally { @@ -4234,7 +4226,7 @@ public void enableServiceConfigLookUp_resolverReturnsNoConfig_withDefaultConfig( createChannel(); ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).containsExactly(addressGroup); verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class)); } finally { @@ -4260,7 +4252,7 @@ public void enableServiceConfigLookUp_resolverReturnsNoConfig_noDefaultConfig() createChannel(); ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAddresses()).containsExactly(addressGroup); verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class)); } finally { @@ -4340,7 +4332,7 @@ public void healthCheckingConfigPropagated() throws Exception { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); assertThat(resultCaptor.getValue().getAttributes() .get(LoadBalancer.ATTR_HEALTH_CHECKING_CONFIG)) .containsExactly("serviceName", "service1"); diff --git a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java index 6abb477878a..f592ebc9b37 100644 --- a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java +++ b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java @@ -192,7 +192,7 @@ public void run() { @Before public void setUp() throws Exception { - when(mockLoadBalancer.canHandleEmptyAddressListFromNameResolution()).thenCallRealMethod(); + mockLoadBalancer.setAcceptAddresses(true); LoadBalancerRegistry.getDefaultRegistry().register(mockLoadBalancerProvider); expectedUri = new URI(TARGET); when(mockTransportFactory.getScheduledExecutorService()) @@ -268,7 +268,7 @@ public void emptyAddresses_validConfig_2ndResolution_lbNeedsAddress() throws Exc ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); ResolvedAddresses resolvedAddresses = resultCaptor.getValue(); assertThat(resolvedAddresses.getAddresses()).containsExactly(addressGroup); assertThat(resolvedAddresses.getLoadBalancingPolicyConfig()).isEqualTo("12"); @@ -280,19 +280,14 @@ public void emptyAddresses_validConfig_2ndResolution_lbNeedsAddress() throws Exc nameResolverFactory.servers.clear(); // 2nd resolution + mockLoadBalancer.setAcceptAddresses(false); nameResolverFactory.allResolved(); // 2nd service config without addresses - ArgumentCaptor statusCaptor = ArgumentCaptor.forClass(Status.class); - verify(mockLoadBalancer, never()).handleResolvedAddresses(any(ResolvedAddresses.class)); - verify(mockLoadBalancer).handleNameResolutionError(statusCaptor.capture()); - assertThat(statusCaptor.getValue().getCode()).isEqualTo(Status.Code.UNAVAILABLE); - assertThat(statusCaptor.getValue().getDescription()) - .contains("NameResolver returned no usable address."); - assertThat(channel.getState(true)).isEqualTo(ConnectivityState.TRANSIENT_FAILURE); - assertWithMessage("Empty address should schedule NameResolver retry") - .that(getNameResolverRefresh()) - .isNotNull(); + verify(mockLoadBalancer).acceptResolvedAddresses(any(ResolvedAddresses.class)); + + // A resolution retry has been scheduled + assertEquals(1, timer.numPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER)); } @Test @@ -302,7 +297,6 @@ public void emptyAddresses_validConfig_lbDoesNotNeedAddress() throws Exception { .setServers(Collections.emptyList()) .build(); channelBuilder.nameResolverFactory(nameResolverFactory); - when(mockLoadBalancer.canHandleEmptyAddressListFromNameResolution()).thenReturn(true); Map rawServiceConfig = parseJson("{\"loadBalancingConfig\": [{\"mock_lb\": {\"check\": \"val\"}}]}"); @@ -312,7 +306,7 @@ public void emptyAddresses_validConfig_lbDoesNotNeedAddress() throws Exception { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); ResolvedAddresses resolvedAddresses = resultCaptor.getValue(); assertThat(resolvedAddresses.getAddresses()).isEmpty(); @@ -338,7 +332,7 @@ public void validConfig_lbDoesNotNeedAddress() throws Exception { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); ResolvedAddresses resolvedAddresses = resultCaptor.getValue(); assertThat(resolvedAddresses.getAddresses()).containsExactly(addressGroup); assertThat(resolvedAddresses.getLoadBalancingPolicyConfig()).isEqualTo("foo"); @@ -360,7 +354,7 @@ public void noConfig_noDefaultConfig() { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); ResolvedAddresses resolvedAddresses = resultCaptor.getValue(); assertThat(resolvedAddresses.getAddresses()).containsExactly(addressGroup); assertThat(resolvedAddresses.getLoadBalancingPolicyConfig()).isNull(); @@ -386,7 +380,7 @@ public void noConfig_usingDefaultConfig() throws Exception { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); ResolvedAddresses resolvedAddresses = resultCaptor.getValue(); assertThat(resolvedAddresses.getAddresses()).containsExactly(addressGroup); assertThat(resolvedAddresses.getLoadBalancingPolicyConfig()).isEqualTo("foo"); @@ -431,7 +425,7 @@ public void invalidConfig_withDefaultConfig() throws Exception { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); ResolvedAddresses resolvedAddresses = resultCaptor.getValue(); assertThat(resolvedAddresses.getAddresses()).containsExactly(addressGroup); @@ -462,7 +456,7 @@ public void invalidConfig_2ndResolution() throws Exception { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); ResolvedAddresses resolvedAddresses = resultCaptor.getValue(); assertThat(resolvedAddresses.getAddresses()).containsExactly(addressGroup); assertThat(resolvedAddresses.getLoadBalancingPolicyConfig()).isEqualTo("1st raw config"); @@ -477,7 +471,7 @@ public void invalidConfig_2ndResolution() throws Exception { nextLbPolicyConfigError.set(Status.UNKNOWN); nameResolverFactory.allResolved(); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); ResolvedAddresses newResolvedAddress = resultCaptor.getValue(); // should use previous service config because new service config is invalid. assertThat(newResolvedAddress.getLoadBalancingPolicyConfig()).isEqualTo("1st raw config"); @@ -510,7 +504,7 @@ public void validConfig_thenNoConfig_withDefaultConfig() throws Exception { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); ResolvedAddresses resolvedAddresses = resultCaptor.getValue(); assertThat(resolvedAddresses.getAddresses()).containsExactly(addressGroup); // should use previous service config because new resolution result is no config. @@ -522,7 +516,7 @@ public void validConfig_thenNoConfig_withDefaultConfig() throws Exception { nameResolverFactory.nextRawServiceConfig.set(null); nameResolverFactory.allResolved(); - verify(mockLoadBalancer, times(2)).handleResolvedAddresses(resultCaptor.capture()); + verify(mockLoadBalancer, times(2)).acceptResolvedAddresses(resultCaptor.capture()); ResolvedAddresses newResolvedAddress = resultCaptor.getValue(); assertThat(newResolvedAddress.getLoadBalancingPolicyConfig()).isEqualTo("mate"); assertThat(newResolvedAddress.getAttributes().get(InternalConfigSelector.KEY)) @@ -658,6 +652,8 @@ private FakeClock.ScheduledTask getNameResolverRefresh() { private static class FakeLoadBalancer extends LoadBalancer { + private boolean acceptAddresses = true; + @Nullable private Helper helper; @@ -665,6 +661,15 @@ public void setHelper(Helper helper) { this.helper = helper; } + @Override + public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { + return acceptAddresses; + } + + public void setAcceptAddresses(boolean acceptAddresses) { + this.acceptAddresses = acceptAddresses; + } + @Override public void handleNameResolutionError(final Status error) { helper.updateBalancingState(ConnectivityState.TRANSIENT_FAILURE, diff --git a/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java b/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java index 9ac27532ad4..a6e7b7d80c7 100644 --- a/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java +++ b/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java @@ -568,7 +568,7 @@ public LoadBalancer newLoadBalancer(final Helper helper) { LoadBalancer loadBalancer = new LoadBalancer() { @Override - public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { + public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { Map config = (Map) resolvedAddresses.getLoadBalancingPolicyConfig(); if (DEFAULT_TARGET.equals(config.get("target"))) { helper.updateBalancingState( @@ -590,6 +590,8 @@ public PickResult pickSubchannel(PickSubchannelArgs args) { } }); } + + return true; } @Override