From 2448c8b6b9dd019a782c7e3824030e8b650ff700 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 28 Mar 2025 19:49:36 +0000 Subject: [PATCH] util: Replace BUFFER_PICKER with FixedResultPicker I think at some point there were more usages in the tests. But now it is pretty easy. PriorityLb.ChildLbState.picker is initialized to FixedResultPicker(NoResult). So now that GracefulSwitchLb is using the same picker, equals() is able to de-dup an update. --- .../io/grpc/util/GracefulSwitchLoadBalancer.java | 16 +--------------- .../util/GracefulSwitchLoadBalancerTest.java | 7 +++++-- .../io/grpc/xds/PriorityLoadBalancerTest.java | 9 ++++----- 3 files changed, 10 insertions(+), 22 deletions(-) diff --git a/util/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java b/util/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java index e36eec1ff2..1dc4fb6750 100644 --- a/util/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java @@ -19,7 +19,6 @@ package io.grpc.util; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; import com.google.common.base.Objects; import io.grpc.ConnectivityState; @@ -66,19 +65,6 @@ public final class GracefulSwitchLoadBalancer extends ForwardingLoadBalancer { public void shutdown() {} }; - @VisibleForTesting - static final SubchannelPicker BUFFER_PICKER = new SubchannelPicker() { - @Override - public PickResult pickSubchannel(PickSubchannelArgs args) { - return PickResult.withNoResult(); - } - - @Override - public String toString() { - return "BUFFER_PICKER"; - } - }; - private final Helper helper; // While the new policy is not fully switched on, the pendingLb is handling new updates from name @@ -128,7 +114,7 @@ public final class GracefulSwitchLoadBalancer extends ForwardingLoadBalancer { pendingLb = defaultBalancer; pendingBalancerFactory = null; pendingState = ConnectivityState.CONNECTING; - pendingPicker = BUFFER_PICKER; + pendingPicker = new FixedResultPicker(PickResult.withNoResult()); if (newBalancerFactory.equals(currentBalancerFactory)) { return; diff --git a/util/src/test/java/io/grpc/util/GracefulSwitchLoadBalancerTest.java b/util/src/test/java/io/grpc/util/GracefulSwitchLoadBalancerTest.java index 5192d6a2a6..843e16194c 100644 --- a/util/src/test/java/io/grpc/util/GracefulSwitchLoadBalancerTest.java +++ b/util/src/test/java/io/grpc/util/GracefulSwitchLoadBalancerTest.java @@ -21,7 +21,6 @@ import static io.grpc.ConnectivityState.CONNECTING; import static io.grpc.ConnectivityState.IDLE; import static io.grpc.ConnectivityState.READY; import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; -import static io.grpc.util.GracefulSwitchLoadBalancer.BUFFER_PICKER; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.inOrder; @@ -521,7 +520,11 @@ public class GracefulSwitchLoadBalancerTest { helper0.updateBalancingState(CONNECTING, picker); verify(mockHelper, never()).updateBalancingState(CONNECTING, picker); - inOrder.verify(mockHelper).updateBalancingState(CONNECTING, BUFFER_PICKER); + ArgumentCaptor pickerCaptor = ArgumentCaptor.forClass(SubchannelPicker.class); + inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); + assertThat(pickerCaptor.getValue().pickSubchannel(mock(PickSubchannelArgs.class)).hasResult()) + .isFalse(); + inOrder.verify(lb0).shutdown(); // shutdown after update picker = mock(SubchannelPicker.class); diff --git a/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java index 9823501dcd..08d4863d19 100644 --- a/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java @@ -522,8 +522,7 @@ public class PriorityLoadBalancerTest { .setLoadBalancingPolicyConfig(priorityLbConfig) .build()); // Nothing important about this verify, other than to provide a baseline - verify(helper, times(2)) - .updateBalancingState(eq(CONNECTING), pickerReturns(PickResult.withNoResult())); + verify(helper).updateBalancingState(eq(CONNECTING), pickerReturns(PickResult.withNoResult())); assertThat(fooBalancers).hasSize(1); assertThat(fooHelpers).hasSize(1); Helper helper0 = Iterables.getOnlyElement(fooHelpers); @@ -539,7 +538,7 @@ public class PriorityLoadBalancerTest { helper0.updateBalancingState( CONNECTING, EMPTY_PICKER); - verify(helper, times(3)) + verify(helper, times(2)) .updateBalancingState(eq(CONNECTING), pickerReturns(PickResult.withNoResult())); // failover happens @@ -805,7 +804,7 @@ public class PriorityLoadBalancerTest { .setAddresses(ImmutableList.of()) .setLoadBalancingPolicyConfig(priorityLbConfig) .build()); - verify(helper, times(2)).updateBalancingState(eq(CONNECTING), isA(SubchannelPicker.class)); + verify(helper).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. @@ -843,7 +842,7 @@ public class PriorityLoadBalancerTest { .setLoadBalancingPolicyConfig(priorityLbConfig) .build()); - verify(helper, times(6)).updateBalancingState(any(), any()); + verify(helper, times(4)).updateBalancingState(any(), any()); } private void assertLatestConnectivityState(ConnectivityState expectedState) {