From 4743bc8e7b86696dfbba141fdac8089520cdcff4 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 2 Apr 2021 14:20:11 -0700 Subject: [PATCH 01/14] Do not implicitly refresh name resolution when subchannel connection is broken, let LB policies handle it. --- .../io/grpc/internal/ManagedChannelImpl.java | 1 - .../grpc/internal/ManagedChannelImplTest.java | 29 ++++--------------- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index ac9c78ad2f4..971269edb6e 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -1926,7 +1926,6 @@ void onTerminated(InternalSubchannel is) { @Override void onStateChange(InternalSubchannel is, ConnectivityStateInfo newState) { - handleInternalSubchannelState(newState); checkState(listener != null, "listener is null"); listener.onSubchannelState(newState); } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 7f896892d0d..691b4bdf508 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -2095,43 +2095,26 @@ public void lbHelper_getNameResolverRegistry() { .isSameInstanceAs(NameResolverRegistry.getDefaultRegistry()); } - @Test - public void refreshNameResolution_whenSubchannelConnectionFailed_notIdle() { - subtestNameResolutionRefreshWhenConnectionFailed(false, false); - } - @Test public void refreshNameResolution_whenOobChannelConnectionFailed_notIdle() { - subtestNameResolutionRefreshWhenConnectionFailed(true, false); - } - - @Test - public void notRefreshNameResolution_whenSubchannelConnectionFailed_idle() { - subtestNameResolutionRefreshWhenConnectionFailed(false, true); + subtestNameResolutionRefreshWhenConnectionFailed(false); } @Test public void notRefreshNameResolution_whenOobChannelConnectionFailed_idle() { - subtestNameResolutionRefreshWhenConnectionFailed(true, true); + subtestNameResolutionRefreshWhenConnectionFailed(true); } - private void subtestNameResolutionRefreshWhenConnectionFailed( - boolean isOobChannel, boolean isIdle) { + private void subtestNameResolutionRefreshWhenConnectionFailed(boolean isIdle) { FakeNameResolverFactory nameResolverFactory = new FakeNameResolverFactory.Builder(expectedUri) .setServers(Collections.singletonList(new EquivalentAddressGroup(socketAddress))) .build(); channelBuilder.nameResolverFactory(nameResolverFactory); createChannel(); - if (isOobChannel) { - OobChannel oobChannel = (OobChannel) helper.createOobChannel( - Collections.singletonList(addressGroup), "oobAuthority"); - oobChannel.getSubchannel().requestConnection(); - } else { - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); - requestConnectionSafely(helper, subchannel); - } + OobChannel oobChannel = (OobChannel) helper.createOobChannel( + Collections.singletonList(addressGroup), "oobAuthority"); + oobChannel.getSubchannel().requestConnection(); MockClientTransportInfo transportInfo = transports.poll(); assertNotNull(transportInfo); From 13051352f0dbc5982d4c20f52b881ca1e907c107 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 2 Apr 2021 14:21:39 -0700 Subject: [PATCH 02/14] Add subchannel refreshing name resolution when connection broken logic in pick_first. --- .../grpc/internal/PickFirstLoadBalancer.java | 4 ++ .../internal/PickFirstLoadBalancerTest.java | 37 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java index 535b34b015d..d5f74db54a7 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static io.grpc.ConnectivityState.CONNECTING; +import static io.grpc.ConnectivityState.IDLE; import static io.grpc.ConnectivityState.SHUTDOWN; import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; @@ -84,6 +85,9 @@ private void processSubchannelState(Subchannel subchannel, ConnectivityStateInfo if (currentState == SHUTDOWN) { return; } + if (stateInfo.getState() == TRANSIENT_FAILURE || stateInfo.getState() == IDLE) { + helper.refreshNameResolution(); + } SubchannelPicker picker; switch (currentState) { diff --git a/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java b/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java index 1c3e0cb1a1b..720341da0cb 100644 --- a/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java @@ -22,6 +22,8 @@ import static io.grpc.ConnectivityState.READY; import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isA; @@ -160,6 +162,38 @@ public void requestConnectionPicker() throws Exception { verify(mockSubchannel, times(2)).requestConnection(); } + @Test + public void refreshNameResolutionAfterSubchannelConnectionBroken() { + loadBalancer.handleResolvedAddresses( + ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); + verify(mockHelper).createSubchannel(any(CreateSubchannelArgs.class)); + + InOrder inOrder = inOrder(mockHelper, mockSubchannel); + inOrder.verify(mockSubchannel).start(stateListenerCaptor.capture()); + SubchannelStateListener stateListener = stateListenerCaptor.getValue(); + inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); + assertSame(mockSubchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel()); + inOrder.verify(mockSubchannel).requestConnection(); + + stateListener.onSubchannelState(ConnectivityStateInfo.forNonError(CONNECTING)); + inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); + assertNull(pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel()); + Status error = Status.UNAUTHENTICATED.withDescription("permission denied"); + stateListener.onSubchannelState(ConnectivityStateInfo.forTransientFailure(error)); + inOrder.verify(mockHelper).refreshNameResolution(); + inOrder.verify(mockHelper).updateBalancingState(eq(TRANSIENT_FAILURE), pickerCaptor.capture()); + assertEquals(error, pickerCaptor.getValue().pickSubchannel(mockArgs).getStatus()); + stateListener.onSubchannelState(ConnectivityStateInfo.forNonError(READY)); + inOrder.verify(mockHelper).updateBalancingState(eq(READY), pickerCaptor.capture()); + assertSame(mockSubchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel()); + // Simulate receiving go-away so the subchannel transit to IDLE. + stateListener.onSubchannelState(ConnectivityStateInfo.forNonError(IDLE)); + inOrder.verify(mockHelper).refreshNameResolution(); + inOrder.verify(mockHelper).updateBalancingState(eq(IDLE), any(SubchannelPicker.class)); + + verifyNoMoreInteractions(mockHelper, mockSubchannel); + } + @Test public void pickAfterResolvedAndUnchanged() throws Exception { loadBalancer.handleResolvedAddresses( @@ -225,10 +259,12 @@ public void pickAfterStateChangeAfterResolution() throws Exception { Status error = Status.UNAVAILABLE.withDescription("boom!"); stateListener.onSubchannelState(ConnectivityStateInfo.forTransientFailure(error)); + inOrder.verify(mockHelper).refreshNameResolution(); inOrder.verify(mockHelper).updateBalancingState(eq(TRANSIENT_FAILURE), pickerCaptor.capture()); assertEquals(error, pickerCaptor.getValue().pickSubchannel(mockArgs).getStatus()); stateListener.onSubchannelState(ConnectivityStateInfo.forNonError(IDLE)); + inOrder.verify(mockHelper).refreshNameResolution(); inOrder.verify(mockHelper).updateBalancingState(eq(IDLE), pickerCaptor.capture()); assertEquals(Status.OK, pickerCaptor.getValue().pickSubchannel(mockArgs).getStatus()); @@ -294,6 +330,7 @@ public void nameResolutionErrorWithStateChanges() throws Exception { SubchannelStateListener stateListener = stateListenerCaptor.getValue(); stateListener.onSubchannelState(ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE)); + inOrder.verify(mockHelper).refreshNameResolution(); inOrder.verify(mockHelper).updateBalancingState( eq(TRANSIENT_FAILURE), any(SubchannelPicker.class)); From 6c7f2f36d1bd5a73367eae907572f12582e56557 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 2 Apr 2021 14:22:04 -0700 Subject: [PATCH 03/14] Add subchannel refreshing name resolution when connection broken logic in round_robin. --- .../io/grpc/util/RoundRobinLoadBalancer.java | 3 ++ .../grpc/util/RoundRobinLoadBalancerTest.java | 35 +++++++++++++++++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java b/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java index 179755cbf8e..846ed90aecc 100644 --- a/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java +++ b/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java @@ -139,6 +139,9 @@ private void processSubchannelState(Subchannel subchannel, ConnectivityStateInfo if (subchannels.get(stripAttrs(subchannel.getAddresses())) != subchannel) { return; } + if (stateInfo.getState() == TRANSIENT_FAILURE || stateInfo.getState() == IDLE) { + helper.refreshNameResolution(); + } if (stateInfo.getState() == IDLE) { subchannel.requestConnection(); } diff --git a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java index e9f4ff9b439..5def703e7b4 100644 --- a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java @@ -277,11 +277,13 @@ public void pickAfterStateChange() throws Exception { ConnectivityStateInfo.forTransientFailure(error)); assertThat(subchannelStateInfo.value.getState()).isEqualTo(TRANSIENT_FAILURE); assertThat(subchannelStateInfo.value.getStatus()).isEqualTo(error); + inOrder.verify(mockHelper).refreshNameResolution(); inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); assertThat(pickerCaptor.getValue()).isInstanceOf(EmptyPicker.class); deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(IDLE)); + inOrder.verify(mockHelper).refreshNameResolution(); assertThat(subchannelStateInfo.value.getState()).isEqualTo(TRANSIENT_FAILURE); assertThat(subchannelStateInfo.value.getStatus()).isEqualTo(error); @@ -305,9 +307,7 @@ public void stayTransientFailureUntilReady() { deliverSubchannelState( sc, ConnectivityStateInfo.forTransientFailure(error)); - deliverSubchannelState( - sc, - ConnectivityStateInfo.forNonError(IDLE)); + inOrder.verify(mockHelper).refreshNameResolution(); deliverSubchannelState( sc, ConnectivityStateInfo.forNonError(CONNECTING)); @@ -330,6 +330,35 @@ public void stayTransientFailureUntilReady() { verifyNoMoreInteractions(mockHelper); } + @Test + public void refreshNameResolutionWhenSubchannelConnectionBroken() { + InOrder inOrder = inOrder(mockHelper); + loadBalancer.handleResolvedAddresses( + ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(Attributes.EMPTY) + .build()); + + verify(mockHelper, times(3)).createSubchannel(any(CreateSubchannelArgs.class)); + inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), isA(EmptyPicker.class)); + + // Simulate state transitions for each subchannel individually. + for (Subchannel sc : loadBalancer.getSubchannels()) { + verify(sc).requestConnection(); + deliverSubchannelState(sc, ConnectivityStateInfo.forNonError(CONNECTING)); + Status error = Status.UNKNOWN.withDescription("connection broken"); + deliverSubchannelState(sc, ConnectivityStateInfo.forTransientFailure(error)); + inOrder.verify(mockHelper).refreshNameResolution(); + deliverSubchannelState(sc, ConnectivityStateInfo.forNonError(READY)); + inOrder.verify(mockHelper).updateBalancingState(eq(READY), isA(ReadyPicker.class)); + // Simulate receiving go-away so READY subchannels transit to IDLE. + deliverSubchannelState(sc, ConnectivityStateInfo.forNonError(IDLE)); + inOrder.verify(mockHelper).refreshNameResolution(); + verify(sc, times(2)).requestConnection(); + inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), isA(EmptyPicker.class)); + } + + verifyNoMoreInteractions(mockHelper); + } + @Test public void pickerRoundRobin() throws Exception { Subchannel subchannel = mock(Subchannel.class); From b6633e6e88de61292c86a2129fe33b2c2bf414a4 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 2 Apr 2021 14:22:39 -0700 Subject: [PATCH 04/14] Add subchannel refreshing name resolution when connection broken logic in grpclb. --- grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java | 4 ++++ .../test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java index 1c812e14500..95dae44f935 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java +++ b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java @@ -206,6 +206,10 @@ void handleSubchannelState(Subchannel subchannel, ConnectivityStateInfo newState if (config.getMode() == Mode.ROUND_ROBIN && newState.getState() == IDLE) { subchannel.requestConnection(); } + if (newState.getState() == TRANSIENT_FAILURE || newState.getState() == IDLE) { + helper.refreshNameResolution(); + } + AtomicReference stateInfoRef = subchannel.getAttributes().get(STATE_INFO); // If all RR servers are unhealthy, it's possible that at least one connection is CONNECTING at diff --git a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java index edd422a8b76..52b8b9c9906 100644 --- a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java +++ b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java @@ -999,6 +999,7 @@ public void grpclbWorking() { verify(subchannel1).requestConnection(); deliverSubchannelState(subchannel1, ConnectivityStateInfo.forNonError(IDLE)); verify(subchannel1, times(2)).requestConnection(); + inOrder.verify(helper).refreshNameResolution(); inOrder.verify(helper).updateBalancingState(eq(READY), pickerCaptor.capture()); assertThat(logs).containsExactly( "INFO: [grpclb-] Update balancing state to READY: picks=" @@ -1018,6 +1019,7 @@ public void grpclbWorking() { ConnectivityStateInfo errorState1 = ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE.withDescription("error1")); deliverSubchannelState(subchannel1, errorState1); + inOrder.verify(helper).refreshNameResolution(); inOrder.verifyNoMoreInteractions(); // If no subchannel is READY, some with error and the others are IDLE, will report CONNECTING @@ -1179,7 +1181,9 @@ public void roundRobinMode_subchannelStayTransientFailureUntilReady() { // Switch all subchannels to TRANSIENT_FAILURE, making the general state TRANSIENT_FAILURE too. Status error = Status.UNAVAILABLE.withDescription("error"); deliverSubchannelState(subchannel1, ConnectivityStateInfo.forTransientFailure(error)); + inOrder.verify(helper).refreshNameResolution(); deliverSubchannelState(subchannel2, ConnectivityStateInfo.forTransientFailure(error)); + inOrder.verify(helper).refreshNameResolution(); inOrder.verify(helper).updateBalancingState(eq(TRANSIENT_FAILURE), pickerCaptor.capture()); assertThat(((RoundRobinPicker) pickerCaptor.getValue()).pickList) .containsExactly(new ErrorEntry(error)); @@ -1187,6 +1191,7 @@ public void roundRobinMode_subchannelStayTransientFailureUntilReady() { // Switch subchannel1 to IDLE, then to CONNECTING, which are ignored since the previous // subchannel state is TRANSIENT_FAILURE. General state is unchanged. deliverSubchannelState(subchannel1, ConnectivityStateInfo.forNonError(IDLE)); + inOrder.verify(helper).refreshNameResolution(); deliverSubchannelState(subchannel1, ConnectivityStateInfo.forNonError(CONNECTING)); inOrder.verifyNoMoreInteractions(); @@ -1501,11 +1506,13 @@ private void subtestGrpclbFallbackConnectionLost( lbResponseObserver = lbResponseObserverCaptor.getValue(); assertEquals(1, lbRequestObservers.size()); lbRequestObserver = lbRequestObservers.poll(); + inOrder.verify(helper).refreshNameResolution(); } if (allSubchannelsBroken) { for (Subchannel subchannel : subchannels) { // A READY subchannel transits to IDLE when receiving a go-away deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(IDLE)); + inOrder.verify(helper).refreshNameResolution(); } } @@ -1518,6 +1525,7 @@ private void subtestGrpclbFallbackConnectionLost( // connections are lost for (Subchannel subchannel : subchannels) { deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(IDLE)); + inOrder.verify(helper).refreshNameResolution(); } // Exit fallback mode or cancel fallback timer when receiving a new server list from balancer From 8f3f060195d3b9e97b15c3b26624d6743d862053 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 2 Apr 2021 14:23:20 -0700 Subject: [PATCH 05/14] Fix mock in RLS test. --- rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java b/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java index f3a655289d7..3b7e84bd543 100644 --- a/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java @@ -211,6 +211,7 @@ public void lb_working_withDefaultTarget() throws Exception { // search subchannel is down, rescue subchannel is connecting searchSubchannel.updateState(ConnectivityStateInfo.forTransientFailure(Status.NOT_FOUND)); + inOrder.verify(helper) .updateBalancingState(eq(ConnectivityState.CONNECTING), pickerCaptor.capture()); @@ -474,6 +475,11 @@ public void updateBalancingState( // no-op } + @Override + public void refreshNameResolution() { + // no-op + } + @Override public String getAuthority() { return "fake-bigtable.googleapis.com"; From a3b5e137062c31988069778d430179d972bcb2d6 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 9 Apr 2021 11:20:13 -0700 Subject: [PATCH 06/14] Add subchannel refreshing name resolution when connection broken logic in ring hash LB policy. --- xds/src/main/java/io/grpc/xds/RingHashLoadBalancer.java | 3 +++ xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancer.java b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancer.java index bdbaa238e21..6ca61f1bdf4 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancer.java +++ b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancer.java @@ -203,6 +203,9 @@ private void processSubchannelState(Subchannel subchannel, ConnectivityStateInfo if (subchannels.get(stripAttrs(subchannel.getAddresses())) != subchannel) { return; } + if (stateInfo.getState() == TRANSIENT_FAILURE || stateInfo.getState() == IDLE) { + helper.refreshNameResolution(); + } Ref subchannelStateRef = getSubchannelStateInfoRef(subchannel); // Don't proactively reconnect if the subchannel enters IDLE, even if previously was connected. diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java index b85b98a089e..231dbbe4f8c 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java @@ -233,12 +233,14 @@ public void aggregateSubchannelStates_connectingReadyIdleFailure() { subchannels.get(Collections.singletonList(servers.get(0))), ConnectivityStateInfo.forTransientFailure( Status.UNKNOWN.withDescription("unknown failure"))); + inOrder.verify(helper).refreshNameResolution(); inOrder.verify(helper).updateBalancingState(eq(READY), any(SubchannelPicker.class)); // one in TRANSIENT_FAILURE, one in IDLE deliverSubchannelState( subchannels.get(Collections.singletonList(servers.get(1))), ConnectivityStateInfo.forNonError(IDLE)); + inOrder.verify(helper).refreshNameResolution(); inOrder.verify(helper).updateBalancingState(eq(IDLE), any(SubchannelPicker.class)); verifyNoMoreInteractions(helper); @@ -260,6 +262,7 @@ public void aggregateSubchannelStates_twoOrMoreSubchannelsInTransientFailure() { subchannels.get(Collections.singletonList(servers.get(0))), ConnectivityStateInfo.forTransientFailure( Status.UNAVAILABLE.withDescription("not found"))); + inOrder.verify(helper).refreshNameResolution(); inOrder.verify(helper).updateBalancingState(eq(IDLE), any(SubchannelPicker.class)); // two in TRANSIENT_FAILURE, two in IDLE @@ -267,6 +270,7 @@ public void aggregateSubchannelStates_twoOrMoreSubchannelsInTransientFailure() { subchannels.get(Collections.singletonList(servers.get(1))), ConnectivityStateInfo.forTransientFailure( Status.UNAVAILABLE.withDescription("also not found"))); + inOrder.verify(helper).refreshNameResolution(); inOrder.verify(helper) .updateBalancingState(eq(TRANSIENT_FAILURE), any(SubchannelPicker.class)); @@ -283,6 +287,7 @@ public void aggregateSubchannelStates_twoOrMoreSubchannelsInTransientFailure() { subchannels.get(Collections.singletonList(servers.get(3))), ConnectivityStateInfo.forTransientFailure( Status.UNAVAILABLE.withDescription("connection lost"))); + inOrder.verify(helper).refreshNameResolution(); inOrder.verify(helper) .updateBalancingState(eq(TRANSIENT_FAILURE), any(SubchannelPicker.class)); @@ -311,6 +316,7 @@ public void subchannelStayInTransientFailureUntilBecomeReady() { deliverSubchannelState(subchannel, ConnectivityStateInfo.forTransientFailure( Status.UNAUTHENTICATED.withDescription("Permission denied"))); } + verify(helper, times(3)).refreshNameResolution(); // Stays in IDLE when until there are two or more subchannels in TRANSIENT_FAILURE. verify(helper).updateBalancingState(eq(IDLE), any(SubchannelPicker.class)); From be2386c6da66c340d52a1c9770cc845335a4f8b4 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 15 Apr 2021 11:52:16 -0700 Subject: [PATCH 07/14] Add ignoreRefreshNameResolutionCheck() API to skip warning and Channel's refresh for cases LoadBalancer intentionally does not want a refresh. --- api/src/main/java/io/grpc/LoadBalancer.java | 16 ++++++++++++++++ .../grpc/util/ForwardingLoadBalancerHelper.java | 6 ++++++ 2 files changed, 22 insertions(+) diff --git a/api/src/main/java/io/grpc/LoadBalancer.java b/api/src/main/java/io/grpc/LoadBalancer.java index 04431fbd620..a2bbfcedde2 100644 --- a/api/src/main/java/io/grpc/LoadBalancer.java +++ b/api/src/main/java/io/grpc/LoadBalancer.java @@ -1059,6 +1059,22 @@ public void refreshNameResolution() { throw new UnsupportedOperationException(); } + /** + * Historically the channel automatically refreshes name resolution if any subchannel + * connection is broken. It's transitioning to let load balancers make the decision. To + * avoid silent breakages, the channel checks if {@link #refreshNameResolution} is called + * by the load balancer. If not, it will do it and log a warning. This will be removed in + * the future and load balancers are completely responsible for triggering the refresh. + * See #8088 for the background. + * + * @deprecated Do NOT use. + */ + @Deprecated + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/8088") + public void ignoreRefreshNameResolutionCheck() { + throw new UnsupportedOperationException(); + } + /** * Returns a {@link SynchronizationContext} that runs tasks in the same Synchronization Context * as that the callback methods on the {@link LoadBalancer} interface are run in. diff --git a/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java b/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java index 42299571f84..5ccccf41317 100644 --- a/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java +++ b/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java @@ -94,6 +94,12 @@ public void refreshNameResolution() { delegate().refreshNameResolution(); } + @Deprecated + @Override + public void ignoreRefreshNameResolutionCheck() { + delegate().ignoreRefreshNameResolutionCheck(); + } + @Override public String getAuthority() { return delegate().getAuthority(); From fd1b608504420d97ed17a56394fc926cda1c4279 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 15 Apr 2021 11:53:51 -0700 Subject: [PATCH 08/14] Check if refreshNameResolution() has been called by the LoadBalancer when handling subchannel state changes. Log a warning and do a refresh if it is not. --- .../io/grpc/internal/ManagedChannelImpl.java | 19 +++ .../grpc/internal/ManagedChannelImplTest.java | 117 ++++++++++++++++++ 2 files changed, 136 insertions(+) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 971269edb6e..7d0c65cf0bb 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -1415,6 +1415,8 @@ void remove(RetriableStream retriableStream) { private final class LbHelperImpl extends LoadBalancer.Helper { AutoConfiguredLoadBalancer lb; + boolean nsRefreshedByLb; + boolean ignoreRefreshNsCheck; @Override public AbstractSubchannel createSubchannel(CreateSubchannelArgs args) { @@ -1453,6 +1455,7 @@ public void run() { @Override public void refreshNameResolution() { syncContext.throwIfNotInThisSynchronizationContext(); + nsRefreshedByLb = true; final class LoadBalancerRefreshNameResolution implements Runnable { @Override public void run() { @@ -1463,6 +1466,12 @@ public void run() { syncContext.execute(new LoadBalancerRefreshNameResolution()); } + @Deprecated + @Override + public void ignoreRefreshNameResolutionCheck() { + ignoreRefreshNsCheck = true; + } + @Override public ManagedChannel createOobChannel(EquivalentAddressGroup addressGroup, String authority) { return createOobChannel(Collections.singletonList(addressGroup), authority); @@ -1928,6 +1937,16 @@ void onTerminated(InternalSubchannel is) { void onStateChange(InternalSubchannel is, ConnectivityStateInfo newState) { checkState(listener != null, "listener is null"); listener.onSubchannelState(newState); + if (newState.getState() == TRANSIENT_FAILURE || newState.getState() == IDLE) { + if (!helper.ignoreRefreshNsCheck && !helper.nsRefreshedByLb) { + logger.log(Level.WARNING, + "LoadBalancer should call Helper.refreshNameResolution() to refresh name " + + "resolution if subchannel connection is broken. This will no longer happen" + + " automatically in the future releases"); + refreshAndResetNameResolution(); + helper.nsRefreshedByLb = true; + } + } } @Override diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 691b4bdf508..192c8d0e449 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -139,6 +139,9 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; import javax.annotation.Nullable; import org.junit.After; import org.junit.Assert; @@ -277,6 +280,7 @@ public String getPolicyName() { private boolean requestConnection = true; private BlockingQueue transports; private boolean panicExpected; + private final List logs = new ArrayList<>(); @Captor private ArgumentCaptor resolvedAddressCaptor; @@ -328,6 +332,22 @@ public void setUp() throws Exception { when(executorPool.getObject()).thenReturn(executor.getScheduledExecutorService()); when(balancerRpcExecutorPool.getObject()) .thenReturn(balancerRpcExecutor.getScheduledExecutorService()); + Handler handler = new Handler() { + @Override + public void publish(LogRecord record) { + logs.add(record); + } + + @Override + public void flush() { + } + + @Override + public void close() throws SecurityException { + } + }; + ManagedChannelImpl.logger.addHandler(handler); + ManagedChannelImpl.logger.setLevel(Level.ALL); channelBuilder = new ManagedChannelImplBuilder(TARGET, new UnsupportedClientTransportFactoryBuilder(), new FixedPortProvider(DEFAULT_PORT)); @@ -1539,6 +1559,103 @@ public void run() { timer.forwardTime(ManagedChannelImpl.SUBCHANNEL_SHUTDOWN_DELAY_SECONDS, TimeUnit.SECONDS); } + @Test + public void subchannelConnectionBroken_noLbRefreshingResolver_logWarningAndTriggeRefresh() { + FakeNameResolverFactory nameResolverFactory = + new FakeNameResolverFactory.Builder(expectedUri) + .setServers(Collections.singletonList(new EquivalentAddressGroup(socketAddress))) + .build(); + channelBuilder.nameResolverFactory(nameResolverFactory); + createChannel(); + FakeNameResolverFactory.FakeNameResolver resolver = + Iterables.getOnlyElement(nameResolverFactory.resolvers); + assertThat(resolver.refreshCalled).isEqualTo(0); + + Subchannel subchannel = + createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + InternalSubchannel internalSubchannel = + (InternalSubchannel) subchannel.getInternalSubchannel(); + internalSubchannel.obtainActiveTransport(); + MockClientTransportInfo transportInfo = transports.poll(); + + // Break subchannel connection + transportInfo.listener.transportShutdown(Status.UNAVAILABLE.withDescription("unreachable")); + LogRecord log = Iterables.getOnlyElement(logs); + assertThat(log.getLevel()).isEqualTo(Level.WARNING); + assertThat(log.getMessage()).isEqualTo( + "LoadBalancer should call Helper.refreshNameResolution() to refresh name resolution if " + + "subchannel connection is broken. This will no longer happen automatically in the " + + "future releases"); + assertThat(resolver.refreshCalled).isEqualTo(1); + } + + @Test + public void subchannelConnectionBroken_ResolverRefreshedByLb() { + FakeNameResolverFactory nameResolverFactory = + new FakeNameResolverFactory.Builder(expectedUri) + .setServers(Collections.singletonList(new EquivalentAddressGroup(socketAddress))) + .build(); + channelBuilder.nameResolverFactory(nameResolverFactory); + createChannel(); + FakeNameResolverFactory.FakeNameResolver resolver = + Iterables.getOnlyElement(nameResolverFactory.resolvers); + assertThat(resolver.refreshCalled).isEqualTo(0); + ArgumentCaptor helperCaptor = ArgumentCaptor.forClass(Helper.class); + verify(mockLoadBalancerProvider).newLoadBalancer(helperCaptor.capture()); + helper = helperCaptor.getValue(); + + SubchannelStateListener listener = new SubchannelStateListener() { + @Override + public void onSubchannelState(ConnectivityStateInfo newState) { + // Normal LoadBalancer should refresh name resolution when some subchannel enters + // TRANSIENT_FAILURE or IDLE + if (newState.getState() == TRANSIENT_FAILURE || newState.getState() == IDLE) { + helper.refreshNameResolution(); + } + } + }; + Subchannel subchannel = + createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, listener); + InternalSubchannel internalSubchannel = + (InternalSubchannel) subchannel.getInternalSubchannel(); + internalSubchannel.obtainActiveTransport(); + MockClientTransportInfo transportInfo = transports.poll(); + + // Break subchannel connection and simulate load balancer refreshing name resolution + transportInfo.listener.transportShutdown(Status.UNAVAILABLE.withDescription("unreachable")); + assertThat(logs).isEmpty(); + assertThat(resolver.refreshCalled).isEqualTo(1); + } + + @Test + public void subchannelConnectionBroken_ignoreRefreshNameResolutionCheck_noRefresh() { + FakeNameResolverFactory nameResolverFactory = + new FakeNameResolverFactory.Builder(expectedUri) + .setServers(Collections.singletonList(new EquivalentAddressGroup(socketAddress))) + .build(); + channelBuilder.nameResolverFactory(nameResolverFactory); + createChannel(); + FakeNameResolverFactory.FakeNameResolver resolver = + Iterables.getOnlyElement(nameResolverFactory.resolvers); + assertThat(resolver.refreshCalled).isEqualTo(0); + ArgumentCaptor helperCaptor = ArgumentCaptor.forClass(Helper.class); + verify(mockLoadBalancerProvider).newLoadBalancer(helperCaptor.capture()); + helper = helperCaptor.getValue(); + helper.ignoreRefreshNameResolutionCheck(); + + Subchannel subchannel = + createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + InternalSubchannel internalSubchannel = + (InternalSubchannel) subchannel.getInternalSubchannel(); + internalSubchannel.obtainActiveTransport(); + MockClientTransportInfo transportInfo = transports.poll(); + + // Break subchannel connection + transportInfo.listener.transportShutdown(Status.UNAVAILABLE.withDescription("unreachable")); + assertThat(logs).isEmpty(); + assertThat(resolver.refreshCalled).isEqualTo(0); + } + @Test public void subchannelStringableBeforeStart() { createChannel(); From 2ff23c69afef82cd188ab762efdba6c80b33ee24 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 15 Apr 2021 11:55:20 -0700 Subject: [PATCH 09/14] The cluster_resolver LB policy intentionally does not want to trigger a name resolution refresh for the resolver in Channel, so use ignoreRefreshNameResolutionCheck() to avoid false-positive warnings. --- .../main/java/io/grpc/xds/ClusterResolverLoadBalancer.java | 2 ++ .../java/io/grpc/xds/ClusterResolverLoadBalancerTest.java | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancer.java b/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancer.java index 4ee0e773a2d..37dc4e741a8 100644 --- a/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancer.java +++ b/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancer.java @@ -279,8 +279,10 @@ private void handleEndpointResolutionError() { private final class RefreshableHelper extends ForwardingLoadBalancerHelper { private final Helper delegate; + @SuppressWarnings("deprecation") private RefreshableHelper(Helper delegate) { this.delegate = checkNotNull(delegate, "delegate"); + delegate.ignoreRefreshNameResolutionCheck(); } @Override diff --git a/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java index 3c2c226cd4f..15b4d9c52b6 100644 --- a/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java @@ -452,6 +452,7 @@ public void onlyLogicalDnsCluster_endpointsResolved() { assertAddressesEqual(Arrays.asList(endpoint1, endpoint2), childBalancer.addresses); } + @SuppressWarnings("deprecation") @Test public void onlyLogicalDnsCluster_handleRefreshNameResolution() { deliverConfigWithSingleLogicalDnsCluster(); @@ -460,6 +461,7 @@ public void onlyLogicalDnsCluster_handleRefreshNameResolution() { FakeNameResolver resolver = Iterables.getOnlyElement(resolvers); resolver.deliverEndpointAddresses(Arrays.asList(endpoint1, endpoint2)); assertThat(resolver.refreshCount).isEqualTo(0); + verify(helper).ignoreRefreshNameResolutionCheck(); FakeLoadBalancer childBalancer = Iterables.getOnlyElement(childBalancers); childBalancer.helper.refreshNameResolution(); assertThat(resolver.refreshCount).isEqualTo(1); @@ -509,6 +511,7 @@ public void onlyLogicalDnsCluster_resolutionError_backoffAndRefresh() { inOrder.verifyNoMoreInteractions(); } + @SuppressWarnings("deprecation") @Test public void onlyLogicalDnsCluster_refreshNameResolutionRaceWithResolutionError() { InOrder inOrder = Mockito.inOrder(backoffPolicyProvider, backoffPolicy1, backoffPolicy2); @@ -519,6 +522,7 @@ public void onlyLogicalDnsCluster_refreshNameResolutionRaceWithResolutionError() FakeLoadBalancer childBalancer = Iterables.getOnlyElement(childBalancers); assertAddressesEqual(Collections.singletonList(endpoint), childBalancer.addresses); assertThat(resolver.refreshCount).isEqualTo(0); + verify(helper).ignoreRefreshNameResolutionCheck(); childBalancer.helper.refreshNameResolution(); assertThat(resolver.refreshCount).isEqualTo(1); From cf5af02e1d10509478b4aa10b80b123df29a3bdf Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 15 Apr 2021 12:36:25 -0700 Subject: [PATCH 10/14] Add proper annotation. --- core/src/main/java/io/grpc/internal/ManagedChannelImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 7d0c65cf0bb..6388c39d5d1 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -117,6 +117,7 @@ @ThreadSafe final class ManagedChannelImpl extends ManagedChannel implements InternalInstrumented { + @VisibleForTesting static final Logger logger = Logger.getLogger(ManagedChannelImpl.class.getName()); // Matching this pattern means the target string is a URI target or at least intended to be one. From e443a78a364df3bd0a6f364641b28301904b626c Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 15 Apr 2021 16:38:25 -0700 Subject: [PATCH 11/14] Improve warning message. --- core/src/main/java/io/grpc/internal/ManagedChannelImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 6388c39d5d1..b814f943ca9 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -1942,8 +1942,8 @@ void onStateChange(InternalSubchannel is, ConnectivityStateInfo newState) { if (!helper.ignoreRefreshNsCheck && !helper.nsRefreshedByLb) { logger.log(Level.WARNING, "LoadBalancer should call Helper.refreshNameResolution() to refresh name " - + "resolution if subchannel connection is broken. This will no longer happen" - + " automatically in the future releases"); + + "resolution if subchannel state becomes TRANSIENT_FAILURE or IDLE. " + + "This will no longer happen automatically in the future releases"); refreshAndResetNameResolution(); helper.nsRefreshedByLb = true; } From 417532f6c9496eaf33a981bc3cf5a4a498823b41 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 15 Apr 2021 16:47:35 -0700 Subject: [PATCH 12/14] Make LoadBalancer.Helper.ignoreRefreshNameResolutionCheck() default to no-op, update its doc and not mark it as deprecated for now. --- api/src/main/java/io/grpc/LoadBalancer.java | 10 +++++++--- .../main/java/io/grpc/internal/ManagedChannelImpl.java | 1 - .../io/grpc/util/ForwardingLoadBalancerHelper.java | 1 - .../io/grpc/xds/ClusterResolverLoadBalancerTest.java | 2 -- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/io/grpc/LoadBalancer.java b/api/src/main/java/io/grpc/LoadBalancer.java index a2bbfcedde2..4a39ce9d40f 100644 --- a/api/src/main/java/io/grpc/LoadBalancer.java +++ b/api/src/main/java/io/grpc/LoadBalancer.java @@ -1067,12 +1067,16 @@ public void refreshNameResolution() { * the future and load balancers are completely responsible for triggering the refresh. * See #8088 for the background. * - * @deprecated Do NOT use. + *

This should rarely be used, but sometimes the address for the subchannel wasn't + * provided by the name resolver and a refresh needs to be directed somewhere else instead. + * Then you can call this method to disable the short-tem check for detecting LoadBalancers + * that need to be updated for the new expected behavior. + * + * @since 1.38.0 */ - @Deprecated @ExperimentalApi("https://github.com/grpc/grpc-java/issues/8088") public void ignoreRefreshNameResolutionCheck() { - throw new UnsupportedOperationException(); + // no-op } /** diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index b814f943ca9..7c4fab0a610 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -1467,7 +1467,6 @@ public void run() { syncContext.execute(new LoadBalancerRefreshNameResolution()); } - @Deprecated @Override public void ignoreRefreshNameResolutionCheck() { ignoreRefreshNsCheck = true; diff --git a/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java b/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java index 5ccccf41317..05db207806d 100644 --- a/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java +++ b/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java @@ -94,7 +94,6 @@ public void refreshNameResolution() { delegate().refreshNameResolution(); } - @Deprecated @Override public void ignoreRefreshNameResolutionCheck() { delegate().ignoreRefreshNameResolutionCheck(); diff --git a/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java index 15b4d9c52b6..264f6232c0c 100644 --- a/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java @@ -452,7 +452,6 @@ public void onlyLogicalDnsCluster_endpointsResolved() { assertAddressesEqual(Arrays.asList(endpoint1, endpoint2), childBalancer.addresses); } - @SuppressWarnings("deprecation") @Test public void onlyLogicalDnsCluster_handleRefreshNameResolution() { deliverConfigWithSingleLogicalDnsCluster(); @@ -511,7 +510,6 @@ public void onlyLogicalDnsCluster_resolutionError_backoffAndRefresh() { inOrder.verifyNoMoreInteractions(); } - @SuppressWarnings("deprecation") @Test public void onlyLogicalDnsCluster_refreshNameResolutionRaceWithResolutionError() { InOrder inOrder = Mockito.inOrder(backoffPolicyProvider, backoffPolicy1, backoffPolicy2); From 522d545d63c8d6ceaed5b5c20e96e6954eba782a Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 15 Apr 2021 16:53:10 -0700 Subject: [PATCH 13/14] Add TODO note for changing LB policies manage name resolution refresh for OOB channel state changes. --- core/src/main/java/io/grpc/internal/ManagedChannelImpl.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 7c4fab0a610..f68ee449de6 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -1514,6 +1514,8 @@ void onTerminated(InternalSubchannel is) { @Override void onStateChange(InternalSubchannel is, ConnectivityStateInfo newState) { + // TODO(chengyuanzhang): change to let LB policies explicitly manage OOB channel's + // state and refresh name resolution if necessary. handleInternalSubchannelState(newState); oobChannel.handleSubchannelStateChange(newState); } From 8312c614f440a5d6e05b74858a258e7f97f5c522 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 15 Apr 2021 17:04:38 -0700 Subject: [PATCH 14/14] Fix warning message check test. --- .../test/java/io/grpc/internal/ManagedChannelImplTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 192c8d0e449..c0868baeecb 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -1584,8 +1584,8 @@ public void subchannelConnectionBroken_noLbRefreshingResolver_logWarningAndTrigg assertThat(log.getLevel()).isEqualTo(Level.WARNING); assertThat(log.getMessage()).isEqualTo( "LoadBalancer should call Helper.refreshNameResolution() to refresh name resolution if " - + "subchannel connection is broken. This will no longer happen automatically in the " - + "future releases"); + + "subchannel state becomes TRANSIENT_FAILURE or IDLE. This will no longer happen " + + "automatically in the future releases"); assertThat(resolver.refreshCalled).isEqualTo(1); }