From 066e72dc5dccf7b61328ca3d261ad6123bd930f7 Mon Sep 17 00:00:00 2001 From: ZHANG Dapeng Date: Tue, 14 Jan 2020 09:54:48 -0800 Subject: [PATCH] core/util: Go to TRANSIENT_FAILURE on initial resolution error for GracefulSwitchLoadBalancer (#6598) Go to TRANSIENT_FAILURE immediately instead of NOOP if GracefulSwitchLoadBalancer receives resolution error before switching to any delegate. In most natural usecase, the `gracefulSwitchLb` is a child balancer of some `parentLb`, and the `gracefulSwitchLb` switches to a new `delegateLb` when `parentLb.handleResolvedAddressGroup()`. If the `parentLb` receives a resolution error before receiving any resolved addresses, it should go to TRANSIENT_FAILURE. In this case, it will be more convenient if the initial `gracefulSwitchLb` can go to TRANSIENT_FAILURE directly. --- .../grpc/util/GracefulSwitchLoadBalancer.java | 42 +++++++++++++++---- .../util/GracefulSwitchLoadBalancerTest.java | 14 +++++++ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java b/core/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java index d642ce97cd..cdb68685ca 100644 --- a/core/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java +++ b/core/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java @@ -32,13 +32,39 @@ import javax.annotation.concurrent.NotThreadSafe; * A load balancer that gracefully swaps to a new lb policy. If the channel is currently in a state * other than READY, the new policy will be swapped into place immediately. Otherwise, the channel * will keep using the old policy until the new policy reports READY or the old policy exits READY. + * + *

The balancer must {@link #switchTo(Factory) switch to} a policy prior to {@link + * LoadBalancer#handleResolvedAddresses(ResolvedAddresses) handling resolved addresses} for the + * first time. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/5999") @NotThreadSafe // Must be accessed in SynchronizationContext public final class GracefulSwitchLoadBalancer extends ForwardingLoadBalancer { - private static final LoadBalancer NOOP_BALANCER = new LoadBalancer() { + private final LoadBalancer defaultBalancer = new LoadBalancer() { @Override - public void handleNameResolutionError(Status error) {} + public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { + // Most LB policies using this class will receive child policy configuration within the + // service config, so they are naturally calling switchTo() just before + // handleResolvedAddresses(), within their own handleResolvedAddresses(). If switchTo() is + // not called immediately after construction that does open up potential for bugs in the + // parent policies, where they fail to call switchTo(). So we will use the exception to try + // to notice those bugs quickly, as it will fail very loudly. + throw new IllegalStateException( + "GracefulSwitchLoadBalancer must switch to a load balancing policy before handling" + + " ResolvedAddresses"); + } + + @Override + public void handleNameResolutionError(final Status error) { + helper.updateBalancingState( + ConnectivityState.TRANSIENT_FAILURE, + new SubchannelPicker() { + @Override + public PickResult pickSubchannel(PickSubchannelArgs args) { + return PickResult.withError(error); + } + }); + } @Override public void shutdown() {} @@ -64,9 +90,9 @@ public final class GracefulSwitchLoadBalancer extends ForwardingLoadBalancer { // The current fields are guaranteed to be set after the initial swapTo(). // The pending fields are cleared when it becomes current. @Nullable private LoadBalancer.Factory currentBalancerFactory; - private LoadBalancer currentLb = NOOP_BALANCER; + private LoadBalancer currentLb = defaultBalancer; @Nullable private LoadBalancer.Factory pendingBalancerFactory; - private LoadBalancer pendingLb = NOOP_BALANCER; + private LoadBalancer pendingLb = defaultBalancer; private ConnectivityState pendingState; private SubchannelPicker pendingPicker; @@ -87,7 +113,7 @@ public final class GracefulSwitchLoadBalancer extends ForwardingLoadBalancer { return; } pendingLb.shutdown(); - pendingLb = NOOP_BALANCER; + pendingLb = defaultBalancer; pendingBalancerFactory = null; pendingState = ConnectivityState.CONNECTING; pendingPicker = BUFFER_PICKER; @@ -115,7 +141,7 @@ public final class GracefulSwitchLoadBalancer extends ForwardingLoadBalancer { } } else if (lb == currentLb) { currentLbIsReady = newState == ConnectivityState.READY; - if (!currentLbIsReady && pendingLb != NOOP_BALANCER) { + if (!currentLbIsReady && pendingLb != defaultBalancer) { swap(); // current policy exits READY, so swap } else { helper.updateBalancingState(newState, newPicker); @@ -138,13 +164,13 @@ public final class GracefulSwitchLoadBalancer extends ForwardingLoadBalancer { currentLb.shutdown(); currentLb = pendingLb; currentBalancerFactory = pendingBalancerFactory; - pendingLb = NOOP_BALANCER; + pendingLb = defaultBalancer; pendingBalancerFactory = null; } @Override protected LoadBalancer delegate() { - return pendingLb == NOOP_BALANCER ? currentLb : pendingLb; + return pendingLb == defaultBalancer ? currentLb : pendingLb; } @Override diff --git a/core/src/test/java/io/grpc/util/GracefulSwitchLoadBalancerTest.java b/core/src/test/java/io/grpc/util/GracefulSwitchLoadBalancerTest.java index fb5ed03172..f55d93c205 100644 --- a/core/src/test/java/io/grpc/util/GracefulSwitchLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/util/GracefulSwitchLoadBalancerTest.java @@ -19,7 +19,9 @@ package io.grpc.util; import static com.google.common.truth.Truth.assertThat; import static io.grpc.ConnectivityState.CONNECTING; import static io.grpc.ConnectivityState.READY; +import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; import static io.grpc.util.GracefulSwitchLoadBalancer.BUFFER_PICKER; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; @@ -33,6 +35,7 @@ import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Helper; +import io.grpc.LoadBalancer.PickSubchannelArgs; import io.grpc.LoadBalancer.ResolvedAddresses; import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelPicker; @@ -51,6 +54,7 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.ArgumentCaptor; import org.mockito.InOrder; /** @@ -471,6 +475,16 @@ public class GracefulSwitchLoadBalancerTest { verifyNoMoreInteractions(lb0, lb1); } + @Test + public void transientFailureOnInitialResolutionError() { + gracefulSwitchLb.handleNameResolutionError(Status.DATA_LOSS); + ArgumentCaptor pickerCaptor = ArgumentCaptor.forClass(null); + verify(mockHelper).updateBalancingState(eq(TRANSIENT_FAILURE), pickerCaptor.capture()); + SubchannelPicker picker = pickerCaptor.getValue(); + assertThat(picker.pickSubchannel(mock(PickSubchannelArgs.class)).getStatus().getCode()) + .isEqualTo(Status.Code.DATA_LOSS); + } + @Deprecated @Test public void handleSubchannelState_shouldThrow() {