From 10dc41af7421bb6f9df7372d44896898cb3fb8ba Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Wed, 4 Nov 2020 14:36:12 -0800 Subject: [PATCH] core: round robin should ignore name resolution error for channel state change when there are READY subchannels (#7595) Round robin is keeping use of READY subchannels even if there is name resolution error. However, it moves Channel state to TRANSIENT_ERROR. In hierarchical load balancers, the upstream LB policy may need to aggregate pickers from multiple downstream round_robin LB policy while filtering out non-ready subchannels. It cannot infer if the subchannel can be used just from the SubchannelPicker interface. It relies on the state that the round_robin intends to set channel to. So the change is to match the readiness of the picker/subchannel with the state that round_robin tries to update. It will completely ignore name resolution error if there are READY subchannels. --- core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java | 6 +++--- .../test/java/io/grpc/util/RoundRobinLoadBalancerTest.java | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java b/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java index 1ac1f7ca012..179755cbf8e 100644 --- a/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java +++ b/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java @@ -130,9 +130,9 @@ public void onSubchannelState(ConnectivityStateInfo state) { @Override public void handleNameResolutionError(Status error) { - // ready pickers aren't affected by status changes - updateBalancingState(TRANSIENT_FAILURE, - currentPicker instanceof ReadyPicker ? currentPicker : new EmptyPicker(error)); + if (currentState != READY) { + updateBalancingState(TRANSIENT_FAILURE, new EmptyPicker(error)); + } } private void processSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) { diff --git a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java index bab636f2c36..99a5ba4aedb 100644 --- a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java @@ -381,13 +381,12 @@ public void nameResolutionErrorWithActiveChannels() throws Exception { loadBalancer.handleNameResolutionError(Status.NOT_FOUND.withDescription("nameResolutionError")); verify(mockHelper, times(3)).createSubchannel(any(CreateSubchannelArgs.class)); - verify(mockHelper, times(3)) + verify(mockHelper, times(2)) .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture()); Iterator stateIterator = stateCaptor.getAllValues().iterator(); assertEquals(CONNECTING, stateIterator.next()); assertEquals(READY, stateIterator.next()); - assertEquals(TRANSIENT_FAILURE, stateIterator.next()); LoadBalancer.PickResult pickResult = pickerCaptor.getValue().pickSubchannel(mockArgs); assertEquals(readySubchannel, pickResult.getSubchannel());