From 7787673992d80f93de187de35d9cb67b5dc631f2 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 9 Feb 2024 22:40:04 -0800 Subject: [PATCH] xds: Replace isEquivalentTo with equals in LeastRequest This is similar to the changes to round robin in dca89b25. --- .../io/grpc/xds/LeastRequestLoadBalancer.java | 86 ++++++++----------- .../xds/LeastRequestLoadBalancerTest.java | 61 +++++-------- 2 files changed, 60 insertions(+), 87 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/LeastRequestLoadBalancer.java b/xds/src/main/java/io/grpc/xds/LeastRequestLoadBalancer.java index 127acd9b34..c73004a33e 100644 --- a/xds/src/main/java/io/grpc/xds/LeastRequestLoadBalancer.java +++ b/xds/src/main/java/io/grpc/xds/LeastRequestLoadBalancer.java @@ -28,8 +28,6 @@ import static io.grpc.xds.LeastRequestLoadBalancerProvider.MIN_CHOICE_COUNT; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; -import com.google.common.base.Objects; -import com.google.common.base.Preconditions; import io.grpc.Attributes; import io.grpc.ClientStreamTracer; import io.grpc.ClientStreamTracer.StreamInfo; @@ -46,7 +44,6 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; -import javax.annotation.Nonnull; /** * A {@link LoadBalancer} that provides least request load balancing based on @@ -57,12 +54,9 @@ import javax.annotation.Nonnull; * the "power of two choices" (P2C). */ final class LeastRequestLoadBalancer extends MultiChildLoadBalancer { - private static final Status EMPTY_OK = Status.OK.withDescription("no subchannels ready"); - private static final EmptyPicker EMPTY_LR_PICKER = new EmptyPicker(EMPTY_OK); - private final ThreadSafeRandom random; - private LeastRequestPicker currentPicker = EMPTY_LR_PICKER; + private SubchannelPicker currentPicker = new EmptyPicker(); private int choiceCount = DEFAULT_CHOICE_COUNT; LeastRequestLoadBalancer(Helper helper) { @@ -101,11 +95,6 @@ final class LeastRequestLoadBalancer extends MultiChildLoadBalancer { return addressAcceptanceStatus; } - @Override - protected SubchannelPicker getErrorPicker(Status error) { - return new EmptyPicker(error); - } - /** * Updates picker with the list of active subchannels (state == READY). * @@ -132,7 +121,7 @@ final class LeastRequestLoadBalancer extends MultiChildLoadBalancer { } } if (isConnecting) { - updateBalancingState(CONNECTING, EMPTY_LR_PICKER); + updateBalancingState(CONNECTING, new EmptyPicker()); } else { // Give it all the failing children and let it randomly pick among them updateBalancingState(TRANSIENT_FAILURE, @@ -149,8 +138,8 @@ final class LeastRequestLoadBalancer extends MultiChildLoadBalancer { return new LeastRequestLbState(key, pickFirstLbProvider, policyConfig, initialPicker); } - private void updateBalancingState(ConnectivityState state, LeastRequestPicker picker) { - if (state != currentConnectivityState || !picker.isEquivalentTo(currentPicker)) { + private void updateBalancingState(ConnectivityState state, SubchannelPicker picker) { + if (state != currentConnectivityState || !picker.equals(currentPicker)) { getHelper().updateBalancingState(state, picker); currentConnectivityState = state; currentPicker = picker; @@ -183,21 +172,23 @@ final class LeastRequestLoadBalancer extends MultiChildLoadBalancer { } @VisibleForTesting - abstract static class LeastRequestPicker extends SubchannelPicker { - abstract boolean isEquivalentTo(LeastRequestPicker picker); - } - - @VisibleForTesting - static final class ReadyPicker extends LeastRequestPicker { + static final class ReadyPicker extends SubchannelPicker { private final List childLbStates; // non-empty private final int choiceCount; private final ThreadSafeRandom random; + private final int hashCode; ReadyPicker(List childLbStates, int choiceCount, ThreadSafeRandom random) { checkArgument(!childLbStates.isEmpty(), "empty list"); this.childLbStates = childLbStates; this.choiceCount = choiceCount; this.random = checkNotNull(random, "random"); + + int sum = 0; + for (ChildLbState child : childLbStates) { + sum += child.hashCode(); + } + this.hashCode = sum ^ choiceCount; } @Override @@ -244,49 +235,48 @@ final class LeastRequestLoadBalancer extends MultiChildLoadBalancer { return childLbStates; } - @VisibleForTesting @Override - boolean isEquivalentTo(LeastRequestPicker picker) { - if (!(picker instanceof ReadyPicker)) { + public int hashCode() { + return hashCode; + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof ReadyPicker)) { return false; } - ReadyPicker other = (ReadyPicker) picker; - // the lists cannot contain duplicate subchannels - return other == this - || ((childLbStates.size() == other.childLbStates.size() && new HashSet<>( - childLbStates).containsAll(other.childLbStates)) - && choiceCount == other.choiceCount); + ReadyPicker other = (ReadyPicker) o; + if (other == this) { + return true; + } + // the lists cannot contain duplicate children + return hashCode == other.hashCode + && choiceCount == other.choiceCount + && childLbStates.size() == other.childLbStates.size() + && new HashSet<>(childLbStates).containsAll(other.childLbStates); } } @VisibleForTesting - static final class EmptyPicker extends LeastRequestPicker { - - private final Status status; - - EmptyPicker(@Nonnull Status status) { - this.status = Preconditions.checkNotNull(status, "status"); - } - + static final class EmptyPicker extends SubchannelPicker { @Override public PickResult pickSubchannel(PickSubchannelArgs args) { - return status.isOk() ? PickResult.withNoResult() : PickResult.withError(status); + return PickResult.withNoResult(); } @Override - boolean isEquivalentTo(LeastRequestPicker picker) { - return picker instanceof EmptyPicker && (Objects.equal(status, ((EmptyPicker) picker).status) - || (status.isOk() && ((EmptyPicker) picker).status.isOk())); + public int hashCode() { + return getClass().hashCode(); + } + + @Override + public boolean equals(Object o) { + return o instanceof EmptyPicker; } @Override public String toString() { - return MoreObjects.toStringHelper(EmptyPicker.class).add("status", status).toString(); - } - - @VisibleForTesting - Status getStatus() { - return status; + return MoreObjects.toStringHelper(EmptyPicker.class).toString(); } } diff --git a/xds/src/test/java/io/grpc/xds/LeastRequestLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/LeastRequestLoadBalancerTest.java index a2dc7936b0..88b457860f 100644 --- a/xds/src/test/java/io/grpc/xds/LeastRequestLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/LeastRequestLoadBalancerTest.java @@ -63,7 +63,6 @@ import io.grpc.util.MultiChildLoadBalancer.ChildLbState; import io.grpc.xds.LeastRequestLoadBalancer.EmptyPicker; import io.grpc.xds.LeastRequestLoadBalancer.LeastRequestConfig; import io.grpc.xds.LeastRequestLoadBalancer.LeastRequestLbState; -import io.grpc.xds.LeastRequestLoadBalancer.LeastRequestPicker; import io.grpc.xds.LeastRequestLoadBalancer.ReadyPicker; import java.net.SocketAddress; import java.util.ArrayList; @@ -365,7 +364,7 @@ public class LeastRequestLoadBalancerTest { } inOrder.verify(helper) .updateBalancingState(eq(TRANSIENT_FAILURE), pickerCaptor.capture()); - assertThat(getStatusString((LeastRequestPicker)pickerCaptor.getValue())) + assertThat(getStatusString(pickerCaptor.getValue())) .contains("Status{code=UNKNOWN, description=connection broken"); inOrder.verifyNoMoreInteractions(); @@ -379,38 +378,25 @@ public class LeastRequestLoadBalancerTest { verifyNoMoreInteractions(helper); } - private String getStatusString(LeastRequestPicker picker) { + private String getStatusString(SubchannelPicker picker) { if (picker == null) { return ""; } - if (picker instanceof EmptyPicker) { - if (((EmptyPicker) picker).getStatus() == null) { - return ""; - } - return ((EmptyPicker) picker).getStatus().toString(); - } else if (picker instanceof ReadyPicker) { + if (picker instanceof ReadyPicker) { List childLbStates = ((ReadyPicker)picker).getChildLbStates(); if (childLbStates == null || childLbStates.isEmpty()) { return ""; - }; + } - // Note that this is dependent on PickFirst's picker toString retaining the representation - // of the status, but since it is a test and we don't want to expose this value it seems - // a reasonable tradeoff - String pickerStr = childLbStates.get(0).getCurrentPicker().toString(); - int beg = pickerStr.indexOf(", status=Status{"); - if (beg < 0) { - return ""; - } - int end = pickerStr.indexOf('}', beg); - if (end < 0) { - return ""; - } - return pickerStr.substring(beg + ", status=".length(), end + 1); + picker = childLbStates.get(0).getCurrentPicker(); } - throw new IllegalArgumentException("Unrecognized picker: " + picker); + Status status = picker.pickSubchannel(mockArgs).getStatus(); + if (status == null) { + return ""; + } + return status.toString(); } @Test @@ -508,11 +494,10 @@ public class LeastRequestLoadBalancerTest { @Test public void pickerEmptyList() throws Exception { - SubchannelPicker picker = new EmptyPicker(Status.UNKNOWN); + SubchannelPicker picker = new EmptyPicker(); assertNull(picker.pickSubchannel(mockArgs).getSubchannel()); - assertEquals(Status.UNKNOWN, - picker.pickSubchannel(mockArgs).getStatus()); + assertEquals(Status.OK, picker.pickSubchannel(mockArgs).getStatus()); } @Test @@ -623,9 +608,8 @@ public class LeastRequestLoadBalancerTest { @Test public void internalPickerComparisons() { - EmptyPicker emptyOk1 = new EmptyPicker(Status.OK); - EmptyPicker emptyOk2 = new EmptyPicker(Status.OK.withDescription("different OK")); - EmptyPicker emptyErr = new EmptyPicker(Status.UNKNOWN.withDescription("¯\\_(ツ)_//¯")); + EmptyPicker empty1 = new EmptyPicker(); + EmptyPicker empty2 = new EmptyPicker(); loadBalancer.acceptResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); @@ -641,15 +625,14 @@ public class LeastRequestLoadBalancerTest { ReadyPicker ready5 = new ReadyPicker(Arrays.asList(child2, child1), 2, mockRandom); ReadyPicker ready6 = new ReadyPicker(Arrays.asList(child2, child1), 8, mockRandom); - assertTrue(emptyOk1.isEquivalentTo(emptyOk2)); - assertFalse(emptyOk1.isEquivalentTo(emptyErr)); - assertFalse(ready1.isEquivalentTo(ready2)); - assertTrue(ready1.isEquivalentTo(ready3)); - assertTrue(ready3.isEquivalentTo(ready4)); - assertTrue(ready4.isEquivalentTo(ready5)); - assertFalse(emptyOk1.isEquivalentTo(ready1)); - assertFalse(ready1.isEquivalentTo(emptyOk1)); - assertFalse(ready5.isEquivalentTo(ready6)); + assertTrue(empty1.equals(empty2)); + assertFalse(ready1.equals(ready2)); + assertTrue(ready1.equals(ready3)); + assertTrue(ready3.equals(ready4)); + assertTrue(ready4.equals(ready5)); + assertFalse(empty1.equals(ready1)); + assertFalse(ready1.equals(empty1)); + assertFalse(ready5.equals(ready6)); } @Test