From 53a2d506956b70fd264df2ab75b586de43edbf5c Mon Sep 17 00:00:00 2001
From: Terry Wilson <terrymwilson@gmail.com>
Date: Thu, 8 Sep 2022 08:49:07 -0700
Subject: [PATCH] xds: always update priority LB connectivity state (#9527)

Removes the option of skipping the update of the priority LB state when
the failover timer is pending.

This consistency facilitates a future change weher we delay child LB
status updates if the priority LB is performing an update. The upcoming
priority LB policy gRFC also does not require this update to ever be
skipped.
---
 .../java/io/grpc/xds/PriorityLoadBalancer.java     | 14 +++++---------
 .../java/io/grpc/xds/PriorityLoadBalancerTest.java |  3 ++-
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java b/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java
index 63855a28dd..5cae9139ae 100644
--- a/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java
+++ b/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java
@@ -99,9 +99,7 @@ final class PriorityLoadBalancer extends LoadBalancer {
         children.get(priority).updateResolvedAddresses();
       }
     }
-    // Not to report connecting in case a pending priority bumps up on top of the current READY
-    // priority.
-    tryNextPriority(false);
+    tryNextPriority();
   }
 
   @Override
@@ -128,7 +126,7 @@ final class PriorityLoadBalancer extends LoadBalancer {
     children.clear();
   }
 
-  private void tryNextPriority(boolean reportConnecting) {
+  private void tryNextPriority() {
     for (int i = 0; i < priorityNames.size(); i++) {
       String priority = priorityNames.get(i);
       if (!children.containsKey(priority)) {
@@ -153,9 +151,7 @@ final class PriorityLoadBalancer extends LoadBalancer {
         return;
       }
       if (child.failOverTimer != null && child.failOverTimer.isPending()) {
-        if (reportConnecting) {
-          updateOverallState(priority, CONNECTING, BUFFER_PICKER);
-        }
+        updateOverallState(priority, child.connectivityState, child.picker);
         return; // Give priority i time to connect.
       }
       if (priority.equals(currentPriority) && child.connectivityState != TRANSIENT_FAILURE) {
@@ -216,7 +212,7 @@ final class PriorityLoadBalancer extends LoadBalancer {
             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);
+        tryNextPriority();
       }
     }
 
@@ -324,7 +320,7 @@ final class PriorityLoadBalancer extends LoadBalancer {
               seenReadyOrIdleSinceTransientFailure = false;
               failOverTimer.cancel();
             }
-            tryNextPriority(true);
+            tryNextPriority();
           }
         });
       }
diff --git a/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java
index 47888a7980..6b1fae48f1 100644
--- a/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java
+++ b/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java
@@ -23,6 +23,7 @@ import static io.grpc.ConnectivityState.READY;
 import static io.grpc.ConnectivityState.TRANSIENT_FAILURE;
 import static io.grpc.xds.XdsSubchannelPickers.BUFFER_PICKER;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.isA;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.clearInvocations;
 import static org.mockito.Mockito.doReturn;
@@ -675,7 +676,7 @@ public class PriorityLoadBalancerTest {
             .setAddresses(ImmutableList.<EquivalentAddressGroup>of())
             .setLoadBalancingPolicyConfig(priorityLbConfig)
             .build());
-    verify(helper).updateBalancingState(eq(CONNECTING), eq(BUFFER_PICKER));
+    verify(helper, times(2)).updateBalancingState(eq(CONNECTING), isA(SubchannelPicker.class));
 
     // LB shutdown and subchannel state change can happen simultaneously. If shutdown runs first,
     // any further balancing state update should be ignored.