xds: Do not failoverpriority when IDLE->CONNECTING

xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java

@@ -41,6 +41,7 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
@@ -66,6 +67,7 @@ final class PriorityLoadBalancer extends LoadBalancer {
   private List<String> priorityNames;
   // Config for each priority.
   private Map<String, PriorityChildConfig> priorityConfigs;
+  @Nullable private String currentPriority;
   private ConnectivityState currentConnectivityState;
   private SubchannelPicker currentPicker;
 
@@ -113,7 +115,7 @@ final class PriorityLoadBalancer extends LoadBalancer {
       }
     }
     if (gotoTransientFailure) {
-      updateOverallState(TRANSIENT_FAILURE, new ErrorPicker(error));
+      updateOverallState(null, TRANSIENT_FAILURE, new ErrorPicker(error));
     }
   }
 
@@ -134,14 +136,14 @@ final class PriorityLoadBalancer extends LoadBalancer {
             new ChildLbState(priority, priorityConfigs.get(priority).ignoreReresolution);
         children.put(priority, child);
         child.updateResolvedAddresses();
-        updateOverallState(CONNECTING, BUFFER_PICKER);
+        updateOverallState(priority, CONNECTING, BUFFER_PICKER);
         return; // Give priority i time to connect.
       }
       ChildLbState child = children.get(priority);
       child.reactivate();
       if (child.connectivityState.equals(READY) || child.connectivityState.equals(IDLE)) {
         logger.log(XdsLogLevel.DEBUG, "Shifted to priority {0}", priority);
-        updateOverallState(child.connectivityState, child.picker);
+        updateOverallState(priority, child.connectivityState, child.picker);
         for (int j = i + 1; j < priorityNames.size(); j++) {
           String p = priorityNames.get(j);
           if (children.containsKey(p)) {
@@ -152,20 +154,28 @@ final class PriorityLoadBalancer extends LoadBalancer {
       }
       if (child.failOverTimer != null && child.failOverTimer.isPending()) {
         if (reportConnecting) {
-          updateOverallState(CONNECTING, BUFFER_PICKER);
+          updateOverallState(priority, CONNECTING, BUFFER_PICKER);
         }
         return; // Give priority i time to connect.
       }
+      if (priority.equals(currentPriority) && child.connectivityState != TRANSIENT_FAILURE) {
+        // If the current priority is not changed into TRANSIENT_FAILURE, keep using it.
+        updateOverallState(priority, child.connectivityState, child.picker);
+        return;
+      }
     }
     // TODO(zdapeng): Include error details of each priority.
     logger.log(XdsLogLevel.DEBUG, "All priority failed");
     String lastPriority = priorityNames.get(priorityNames.size() - 1);
     SubchannelPicker errorPicker = children.get(lastPriority).picker;
-    updateOverallState(TRANSIENT_FAILURE, errorPicker);
+    updateOverallState(lastPriority, TRANSIENT_FAILURE, errorPicker);
   }
 
-  private void updateOverallState(ConnectivityState state, SubchannelPicker picker) {
+  private void updateOverallState(
-    if (!state.equals(currentConnectivityState) || !picker.equals(currentPicker)) {
+      @Nullable String priority, ConnectivityState state, SubchannelPicker picker) {
+    if (!Objects.equals(priority, currentPriority) || !state.equals(currentConnectivityState)
+        || !picker.equals(currentPicker)) {
+      currentPriority = priority;
       currentConnectivityState = state;
       currentPicker = picker;
       helper.updateBalancingState(state, picker);
@@ -201,6 +211,7 @@ final class PriorityLoadBalancer extends LoadBalancer {
           picker = new ErrorPicker(
               Status.UNAVAILABLE.withDescription("Connection timeout for priority " + priority));
           logger.log(XdsLogLevel.DEBUG, "Priority {0} failed over to next", priority);
+          currentPriority = null; // reset currentPriority to guarantee failover happen
           tryNextPriority(true);
         }
       }

xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java

@@ -399,6 +399,99 @@ public class PriorityLoadBalancerTest {
     verify(balancer3).shutdown();
   }
 
+  @Test
+  public void idleToConnectingDoesNotTriggerFailOver() {
+    PriorityChildConfig priorityChildConfig0 =
+        new PriorityChildConfig(new PolicySelection(fooLbProvider, new Object()), true);
+    PriorityChildConfig priorityChildConfig1 =
+        new PriorityChildConfig(new PolicySelection(fooLbProvider, new Object()), true);
+    PriorityLbConfig priorityLbConfig =
+        new PriorityLbConfig(
+            ImmutableMap.of("p0", priorityChildConfig0, "p1", priorityChildConfig1),
+            ImmutableList.of("p0", "p1"));
+    priorityLb.handleResolvedAddresses(
+        ResolvedAddresses.newBuilder()
+            .setAddresses(ImmutableList.<EquivalentAddressGroup>of())
+            .setLoadBalancingPolicyConfig(priorityLbConfig)
+            .build());
+    assertThat(fooBalancers).hasSize(1);
+    assertThat(fooHelpers).hasSize(1);
+    Helper helper0 = Iterables.getOnlyElement(fooHelpers);
+
+    // p0 gets IDLE.
+    helper0.updateBalancingState(
+        IDLE,
+        BUFFER_PICKER);
+    assertCurrentPickerIsBufferPicker();
+
+    // p0 goes to CONNECTING
+    helper0.updateBalancingState(
+        IDLE,
+        BUFFER_PICKER);
+    assertCurrentPickerIsBufferPicker();
+
+    // no failover happened
+    assertThat(fooBalancers).hasSize(1);
+    assertThat(fooHelpers).hasSize(1);
+  }
+
+  @Test
+  public void readyToConnectDoesNotFailOverButUpdatesPicker() {
+    PriorityChildConfig priorityChildConfig0 =
+        new PriorityChildConfig(new PolicySelection(fooLbProvider, new Object()), true);
+    PriorityChildConfig priorityChildConfig1 =
+        new PriorityChildConfig(new PolicySelection(fooLbProvider, new Object()), true);
+    PriorityLbConfig priorityLbConfig =
+        new PriorityLbConfig(
+            ImmutableMap.of("p0", priorityChildConfig0, "p1", priorityChildConfig1),
+            ImmutableList.of("p0", "p1"));
+    priorityLb.handleResolvedAddresses(
+        ResolvedAddresses.newBuilder()
+            .setAddresses(ImmutableList.<EquivalentAddressGroup>of())
+            .setLoadBalancingPolicyConfig(priorityLbConfig)
+            .build());
+    assertThat(fooBalancers).hasSize(1);
+    assertThat(fooHelpers).hasSize(1);
+    Helper helper0 = Iterables.getOnlyElement(fooHelpers);
+
+    // p0 gets READY.
+    final Subchannel subchannel0 = mock(Subchannel.class);
+    helper0.updateBalancingState(
+        READY,
+        new SubchannelPicker() {
+          @Override
+          public PickResult pickSubchannel(PickSubchannelArgs args) {
+            return PickResult.withSubchannel(subchannel0);
+          }
+        });
+    assertCurrentPickerPicksSubchannel(subchannel0);
+
+    // p0 goes to CONNECTING
+    helper0.updateBalancingState(
+        IDLE,
+        BUFFER_PICKER);
+    assertCurrentPickerIsBufferPicker();
+
+    // no failover happened
+    assertThat(fooBalancers).hasSize(1);
+    assertThat(fooHelpers).hasSize(1);
+
+    // resolution update without priority change does not trigger failover
+    Attributes.Key<String> fooKey = Attributes.Key.create("fooKey");
+    priorityLb.handleResolvedAddresses(
+        ResolvedAddresses.newBuilder()
+            .setAddresses(ImmutableList.<EquivalentAddressGroup>of())
+            .setLoadBalancingPolicyConfig(priorityLbConfig)
+            .setAttributes(Attributes.newBuilder().set(fooKey, "barVal").build())
+            .build());
+
+    assertCurrentPickerIsBufferPicker();
+
+    // no failover happened
+    assertThat(fooBalancers).hasSize(1);
+    assertThat(fooHelpers).hasSize(1);
+  }
+
   @Test
   public void typicalPriorityFailOverFlowWithIdleUpdate() {
     PriorityChildConfig priorityChildConfig0 =