diff --git a/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java b/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java index c72cef9f637..6e4566de76d 100644 --- a/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java +++ b/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java @@ -320,13 +320,14 @@ public void updateBalancingState(final ConnectivityState newState, if (!children.containsKey(priority)) { return; } + ConnectivityState oldState = connectivityState; connectivityState = newState; picker = newPicker; if (deletionTimer != null && deletionTimer.isPending()) { return; } - if (newState.equals(CONNECTING)) { + if (newState.equals(CONNECTING) && !oldState.equals(newState)) { if (!failOverTimer.isPending() && seenReadyOrIdleSinceTransientFailure) { failOverTimer = syncContext.schedule(new FailOverTask(), 10, TimeUnit.SECONDS, executor); diff --git a/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java index ab4c00398f0..beb568be9ce 100644 --- a/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java @@ -28,6 +28,7 @@ import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -70,6 +71,7 @@ import org.junit.runners.JUnit4; import org.mockito.ArgumentCaptor; import org.mockito.Captor; +import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; @@ -554,6 +556,55 @@ public void connectingResetFailOverIfSeenReadyOrIdleSinceTransientFailure() { assertThat(fooHelpers).hasSize(2); } + @Test + public void failoverTimerNotRestartedOnDupConnecting() { + InOrder inOrder = inOrder(helper); + PriorityChildConfig priorityChildConfig0 = + new PriorityChildConfig(newChildConfig(fooLbProvider, new Object()), true); + PriorityChildConfig priorityChildConfig1 = + new PriorityChildConfig(newChildConfig(fooLbProvider, new Object()), true); + PriorityLbConfig priorityLbConfig = + new PriorityLbConfig( + ImmutableMap.of("p0", priorityChildConfig0, "p1", priorityChildConfig1), + ImmutableList.of("p0", "p1")); + priorityLb.acceptResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.of()) + .setLoadBalancingPolicyConfig(priorityLbConfig) + .build()); + // Nothing important about this verify, other than to provide a baseline + inOrder.verify(helper) + .updateBalancingState(eq(CONNECTING), pickerReturns(PickResult.withNoResult())); + assertThat(fooBalancers).hasSize(1); + assertThat(fooHelpers).hasSize(1); + Helper helper0 = Iterables.getOnlyElement(fooHelpers); + + // Cause seenReadyOrIdleSinceTransientFailure = true + helper0.updateBalancingState(IDLE, EMPTY_PICKER); + inOrder.verify(helper) + .updateBalancingState(eq(IDLE), pickerReturns(PickResult.withNoResult())); + helper0.updateBalancingState(CONNECTING, EMPTY_PICKER); + + // p0 keeps repeating CONNECTING, failover happens + fakeClock.forwardTime(5, TimeUnit.SECONDS); + helper0.updateBalancingState(CONNECTING, EMPTY_PICKER); + fakeClock.forwardTime(5, TimeUnit.SECONDS); + assertThat(fooBalancers).hasSize(2); + assertThat(fooHelpers).hasSize(2); + inOrder.verify(helper, times(2)) + .updateBalancingState(eq(CONNECTING), pickerReturns(PickResult.withNoResult())); + Helper helper1 = Iterables.getLast(fooHelpers); + + // p0 keeps repeating CONNECTING, no reset of failover timer + helper1.updateBalancingState(IDLE, EMPTY_PICKER); // Stop timer for p1 + inOrder.verify(helper) + .updateBalancingState(eq(IDLE), pickerReturns(PickResult.withNoResult())); + helper0.updateBalancingState(CONNECTING, EMPTY_PICKER); + fakeClock.forwardTime(10, TimeUnit.SECONDS); + inOrder.verify(helper, never()) + .updateBalancingState(eq(CONNECTING), any()); + } + @Test public void readyToConnectDoesNotFailOverButUpdatesPicker() { PriorityChildConfig priorityChildConfig0 =