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.
diff --git a/core/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java b/core/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java
index d642ce9..cdb6868 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 @@
* 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.
+ *
+ * <p>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 @@
// 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 @@
return;
}
pendingLb.shutdown();
- pendingLb = NOOP_BALANCER;
+ pendingLb = defaultBalancer;
pendingBalancerFactory = null;
pendingState = ConnectivityState.CONNECTING;
pendingPicker = BUFFER_PICKER;
@@ -115,7 +141,7 @@
}
} 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 @@
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 fb5ed03..f55d93c 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 @@
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.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.rules.ExpectedException;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
+import org.mockito.ArgumentCaptor;
import org.mockito.InOrder;
/**
@@ -471,6 +475,16 @@
verifyNoMoreInteractions(lb0, lb1);
}
+ @Test
+ public void transientFailureOnInitialResolutionError() {
+ gracefulSwitchLb.handleNameResolutionError(Status.DATA_LOSS);
+ ArgumentCaptor<SubchannelPicker> 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() {