From dc83446d982b53f3f64ad885db23571729cd7c9b Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 25 Jul 2024 13:09:13 -0700 Subject: [PATCH] xds: Stop extending RR in WRR They share very little code, and we really don't want RoundRobinLb to be public and non-final. Originally, WRR was expected to share much more code with RR, and even delegated to RR at times. The delegation was removed in 111ff60e. After dca89b25, most of the sharing has been moved out into general-purpose tools that can be used by any LB policy. FixedResultPicker now has equals to makes it as a EmptyPicker replacement. RoundRobinLb still uses EmptyPicker because fixing its tests is a larger change. OutlierDetectionLbTest was changed because FixedResultPicker is used by PickFirstLeafLb, and now RoundRobinLb can squelch some of its updates for ready pickers. --- api/src/main/java/io/grpc/LoadBalancer.java | 14 ++++++ .../io/grpc/util/RoundRobinLoadBalancer.java | 8 ++-- .../OutlierDetectionLoadBalancerTest.java | 4 +- .../xds/WeightedRoundRobinLoadBalancer.java | 45 +++++++++++++++++-- .../WeightedRoundRobinLoadBalancerTest.java | 4 +- 5 files changed, 63 insertions(+), 12 deletions(-) diff --git a/api/src/main/java/io/grpc/LoadBalancer.java b/api/src/main/java/io/grpc/LoadBalancer.java index 80e3f8b89c..15106a5ffc 100644 --- a/api/src/main/java/io/grpc/LoadBalancer.java +++ b/api/src/main/java/io/grpc/LoadBalancer.java @@ -1526,5 +1526,19 @@ public abstract class LoadBalancer { public String toString() { return "FixedResultPicker(" + result + ")"; } + + @Override + public int hashCode() { + return result.hashCode(); + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof FixedResultPicker)) { + return false; + } + FixedResultPicker that = (FixedResultPicker) o; + return this.result.equals(that.result); + } } } diff --git a/util/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java b/util/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java index a06bae545d..7c235bb364 100644 --- a/util/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java @@ -27,7 +27,6 @@ import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import io.grpc.ConnectivityState; import io.grpc.EquivalentAddressGroup; -import io.grpc.Internal; import io.grpc.LoadBalancer; import io.grpc.NameResolver; import java.util.ArrayList; @@ -41,10 +40,9 @@ import java.util.concurrent.atomic.AtomicInteger; * A {@link LoadBalancer} that provides round-robin load-balancing over the {@link * EquivalentAddressGroup}s from the {@link NameResolver}. */ -@Internal -public class RoundRobinLoadBalancer extends MultiChildLoadBalancer { +final class RoundRobinLoadBalancer extends MultiChildLoadBalancer { private final AtomicInteger sequence = new AtomicInteger(new Random().nextInt()); - protected SubchannelPicker currentPicker = new EmptyPicker(); + private SubchannelPicker currentPicker = new EmptyPicker(); public RoundRobinLoadBalancer(Helper helper) { super(helper); @@ -87,7 +85,7 @@ public class RoundRobinLoadBalancer extends MultiChildLoadBalancer { } } - protected SubchannelPicker createReadyPicker(Collection children) { + private SubchannelPicker createReadyPicker(Collection children) { List pickerList = new ArrayList<>(); for (ChildLbState child : children) { SubchannelPicker picker = child.getCurrentPicker(); diff --git a/util/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java b/util/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java index 8af935d813..1b0139affe 100644 --- a/util/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java +++ b/util/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java @@ -569,7 +569,7 @@ public class OutlierDetectionLoadBalancerTest { loadBalancer.acceptResolvedAddresses(buildResolvedAddress(config, servers)); // The PickFirstLeafLB has an extra level of indirection because of health - int expectedStateChanges = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 16 : 12; + int expectedStateChanges = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 8 : 12; generateLoad(ImmutableMap.of(subchannel2, Status.DEADLINE_EXCEEDED), expectedStateChanges); // Move forward in time to a point where the detection timer has fired. @@ -604,7 +604,7 @@ public class OutlierDetectionLoadBalancerTest { assertEjectedSubchannels(ImmutableSet.of(ImmutableSet.copyOf(servers.get(0).getAddresses()))); // Now we produce more load, but the subchannel has started working and is no longer an outlier. - int expectedStateChanges = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 16 : 12; + int expectedStateChanges = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 8 : 12; generateLoad(ImmutableMap.of(), expectedStateChanges); // Move forward in time to a point where the detection timer has fired. diff --git a/xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java b/xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java index c338314807..115857d43f 100644 --- a/xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java +++ b/xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java @@ -42,7 +42,7 @@ import io.grpc.SynchronizationContext.ScheduledHandle; import io.grpc.services.MetricReport; import io.grpc.util.ForwardingLoadBalancerHelper; import io.grpc.util.ForwardingSubchannel; -import io.grpc.util.RoundRobinLoadBalancer; +import io.grpc.util.MultiChildLoadBalancer; import io.grpc.xds.orca.OrcaOobUtil; import io.grpc.xds.orca.OrcaOobUtil.OrcaOobReportListener; import io.grpc.xds.orca.OrcaPerRequestUtil; @@ -90,7 +90,7 @@ import java.util.logging.Logger; * See related documentation: https://cloud.google.com/service-mesh/legacy/load-balancing-apis/proxyless-configure-advanced-traffic-management#custom-lb-config */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/9885") -final class WeightedRoundRobinLoadBalancer extends RoundRobinLoadBalancer { +final class WeightedRoundRobinLoadBalancer extends MultiChildLoadBalancer { private static final LongCounterMetricInstrument RR_FALLBACK_COUNTER; private static final LongCounterMetricInstrument ENDPOINT_WEIGHT_NOT_YET_USEABLE_COUNTER; @@ -107,6 +107,7 @@ final class WeightedRoundRobinLoadBalancer extends RoundRobinLoadBalancer { private final long infTime; private final Ticker ticker; private String locality = ""; + private SubchannelPicker currentPicker = new FixedResultPicker(PickResult.withNoResult()); // The metric instruments are only registered once and shared by all instances of this LB. static { @@ -209,13 +210,51 @@ final class WeightedRoundRobinLoadBalancer extends RoundRobinLoadBalancer { return acceptRetVal.status; } + /** + * Updates picker with the list of active subchannels (state == READY). + */ @Override - public SubchannelPicker createReadyPicker(Collection activeList) { + protected void updateOverallBalancingState() { + List activeList = getReadyChildren(); + if (activeList.isEmpty()) { + // No READY subchannels + + // MultiChildLB will request connection immediately on subchannel IDLE. + boolean isConnecting = false; + for (ChildLbState childLbState : getChildLbStates()) { + ConnectivityState state = childLbState.getCurrentState(); + if (state == ConnectivityState.CONNECTING || state == ConnectivityState.IDLE) { + isConnecting = true; + break; + } + } + + if (isConnecting) { + updateBalancingState( + ConnectivityState.CONNECTING, new FixedResultPicker(PickResult.withNoResult())); + } else { + updateBalancingState( + ConnectivityState.TRANSIENT_FAILURE, createReadyPicker(getChildLbStates())); + } + } else { + updateBalancingState(ConnectivityState.READY, createReadyPicker(activeList)); + } + } + + private SubchannelPicker createReadyPicker(Collection activeList) { return new WeightedRoundRobinPicker(ImmutableList.copyOf(activeList), config.enableOobLoadReport, config.errorUtilizationPenalty, sequence, getHelper(), locality); } + private void updateBalancingState(ConnectivityState state, SubchannelPicker picker) { + if (state != currentConnectivityState || !picker.equals(currentPicker)) { + getHelper().updateBalancingState(state, picker); + currentConnectivityState = state; + currentPicker = picker; + } + } + @VisibleForTesting final class WeightedChildLbState extends ChildLbState { diff --git a/xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java index a5b5651133..dd98f1e1ae 100644 --- a/xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java @@ -536,8 +536,8 @@ public class WeightedRoundRobinLoadBalancerTest { verify(helper, times(3)).createSubchannel( any(CreateSubchannelArgs.class)); verify(helper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); - assertThat(pickerCaptor.getValue().getClass().getName()) - .isEqualTo("io.grpc.util.RoundRobinLoadBalancer$EmptyPicker"); + assertThat(pickerCaptor.getValue().pickSubchannel(mockArgs)) + .isEqualTo(PickResult.withNoResult()); int expectedCount = isEnabledHappyEyeballs() ? servers.size() + 1 : 1; assertThat(fakeClock.forwardTime(11, TimeUnit.SECONDS)).isEqualTo( expectedCount); }