From 203515dd3d3dcc050a3343c6f26780b10dba91ea Mon Sep 17 00:00:00 2001 From: ZHANG Dapeng Date: Thu, 21 Oct 2021 21:24:51 -0700 Subject: [PATCH] rls: fix connectivity state aggregation (#8625) Fix connectivity state aggregation as per http://go/grpc-rls-lb-policy-design#heading=h.6e8tt7xcwcdn > Note that, for the purposes of aggregation, when a child policy reports TRANSIENT_FAILURE, we consider it to continue to be in that state until it reports READY (i.e., we ignore CONNECTING in between the two, no matter how many times it bounces back and forth between TRANSIENT_FAILURE and CONNECTING). --- .../io/grpc/rls/SubchannelStateManager.java | 2 ++ .../io/grpc/rls/SubchannelStateManagerImpl.java | 17 +++++++++++------ .../grpc/rls/ChildLoadBalancerHelperTest.java | 2 +- .../rls/SubchannelStateManagerImplTest.java | 8 ++++++++ 4 files changed, 22 insertions(+), 7 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/SubchannelStateManager.java b/rls/src/main/java/io/grpc/rls/SubchannelStateManager.java index f5437eac75..041d2a817a 100644 --- a/rls/src/main/java/io/grpc/rls/SubchannelStateManager.java +++ b/rls/src/main/java/io/grpc/rls/SubchannelStateManager.java @@ -16,6 +16,7 @@ package io.grpc.rls; +import com.google.common.annotations.VisibleForTesting; import io.grpc.ConnectivityState; import javax.annotation.Nullable; @@ -35,6 +36,7 @@ interface SubchannelStateManager { * {@code null}. */ @Nullable + @VisibleForTesting ConnectivityState getState(String name); /** Returns representative subchannel status from all registered subchannels. */ diff --git a/rls/src/main/java/io/grpc/rls/SubchannelStateManagerImpl.java b/rls/src/main/java/io/grpc/rls/SubchannelStateManagerImpl.java index 6d34e713f0..f2d6d612fd 100644 --- a/rls/src/main/java/io/grpc/rls/SubchannelStateManagerImpl.java +++ b/rls/src/main/java/io/grpc/rls/SubchannelStateManagerImpl.java @@ -30,6 +30,7 @@ final class SubchannelStateManagerImpl implements SubchannelStateManager { private final HashMap stateMap = new HashMap<>(); private final Multiset stateMultiset = HashMultiset.create(); + private ConnectivityState currentState; @Override public void updateState(String name, ConnectivityState newState) { @@ -56,16 +57,20 @@ final class SubchannelStateManagerImpl implements SubchannelStateManager { @Override public ConnectivityState getAggregatedState() { if (stateMultiset.contains(ConnectivityState.READY)) { - return ConnectivityState.READY; + currentState = ConnectivityState.READY; } else if (stateMultiset.contains(ConnectivityState.CONNECTING)) { - return ConnectivityState.CONNECTING; + if (currentState != ConnectivityState.TRANSIENT_FAILURE) { + currentState = ConnectivityState.CONNECTING; + } } else if (stateMultiset.contains(ConnectivityState.IDLE)) { - return ConnectivityState.IDLE; + currentState = ConnectivityState.IDLE; } else if (stateMultiset.contains(ConnectivityState.TRANSIENT_FAILURE)) { - return ConnectivityState.TRANSIENT_FAILURE; + currentState = ConnectivityState.TRANSIENT_FAILURE; + } else { + // empty or shutdown + currentState = ConnectivityState.IDLE; } - // empty or shutdown - return ConnectivityState.IDLE; + return currentState; } @Override diff --git a/rls/src/test/java/io/grpc/rls/ChildLoadBalancerHelperTest.java b/rls/src/test/java/io/grpc/rls/ChildLoadBalancerHelperTest.java index 48901da323..543540f498 100644 --- a/rls/src/test/java/io/grpc/rls/ChildLoadBalancerHelperTest.java +++ b/rls/src/test/java/io/grpc/rls/ChildLoadBalancerHelperTest.java @@ -57,7 +57,7 @@ public class ChildLoadBalancerHelperTest { .isEqualTo(ConnectivityState.TRANSIENT_FAILURE); childLbHelper2.updateBalancingState(ConnectivityState.CONNECTING, picker2); - inOrder.verify(helper).updateBalancingState(ConnectivityState.CONNECTING, picker); + inOrder.verify(helper).updateBalancingState(ConnectivityState.TRANSIENT_FAILURE, picker); assertThat(subchannelStateManager.getState(target2)).isEqualTo(ConnectivityState.CONNECTING); childLbHelper1.updateBalancingState(ConnectivityState.READY, picker1); diff --git a/rls/src/test/java/io/grpc/rls/SubchannelStateManagerImplTest.java b/rls/src/test/java/io/grpc/rls/SubchannelStateManagerImplTest.java index a6d42e7d2a..317d49448e 100644 --- a/rls/src/test/java/io/grpc/rls/SubchannelStateManagerImplTest.java +++ b/rls/src/test/java/io/grpc/rls/SubchannelStateManagerImplTest.java @@ -90,5 +90,13 @@ public class SubchannelStateManagerImplTest { assertThat(subchannelStateManager.getAggregatedState()) .isEqualTo(ConnectivityState.TRANSIENT_FAILURE); + + subchannelStateManager.updateState("channel1", ConnectivityState.CONNECTING); + assertThat(subchannelStateManager.getAggregatedState()) + .isEqualTo(ConnectivityState.TRANSIENT_FAILURE); + + subchannelStateManager.updateState("channel1", ConnectivityState.READY); + assertThat(subchannelStateManager.getAggregatedState()) + .isEqualTo(ConnectivityState.READY); } }