xds: Replace isEquivalentTo with equals in LeastRequest

This is similar to the changes to round robin in dca89b25.

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) {
+  private void updateBalancingState(ConnectivityState state, SubchannelPicker picker) {
-    if (state != currentConnectivityState || !picker.isEquivalentTo(currentPicker)) {
+    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 {
+  static final class ReadyPicker extends SubchannelPicker {
-    abstract boolean isEquivalentTo(LeastRequestPicker picker);
-  }
-
-  @VisibleForTesting
-  static final class ReadyPicker extends LeastRequestPicker {
     private final List<ChildLbState> childLbStates; // non-empty
     private final int choiceCount;
     private final ThreadSafeRandom random;
+    private final int hashCode;
 
     ReadyPicker(List<ChildLbState> 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) {
+    public int hashCode() {
-      if (!(picker instanceof ReadyPicker)) {
+      return hashCode;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (!(o instanceof ReadyPicker)) {
         return false;
       }
-      ReadyPicker other = (ReadyPicker) picker;
+      ReadyPicker other = (ReadyPicker) o;
-      // the lists cannot contain duplicate subchannels
+      if (other == this) {
-      return other == this
+        return true;
-          || ((childLbStates.size() == other.childLbStates.size() && new HashSet<>(
+      }
-          childLbStates).containsAll(other.childLbStates))
+      // the lists cannot contain duplicate children
-                && choiceCount == other.choiceCount);
+      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 {
+  static final class EmptyPicker extends SubchannelPicker {
-
-    private final Status status;
-
-    EmptyPicker(@Nonnull Status status) {
-      this.status = Preconditions.checkNotNull(status, "status");
-    }
-
     @Override
     public PickResult pickSubchannel(PickSubchannelArgs args) {
-      return status.isOk() ? PickResult.withNoResult() : PickResult.withError(status);
+      return PickResult.withNoResult();
     }
 
     @Override
-    boolean isEquivalentTo(LeastRequestPicker picker) {
+    public int hashCode() {
-      return picker instanceof EmptyPicker && (Objects.equal(status, ((EmptyPicker) picker).status)
+      return getClass().hashCode();
-          || (status.isOk() && ((EmptyPicker) picker).status.isOk()));
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      return o instanceof EmptyPicker;
     }
 
     @Override
     public String toString() {
-      return MoreObjects.toStringHelper(EmptyPicker.class).add("status", status).toString();
+      return MoreObjects.toStringHelper(EmptyPicker.class).toString();
-    }
-
-    @VisibleForTesting
-    Status getStatus() {
-      return status;
     }
   }
 

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 (picker instanceof ReadyPicker) {
-      if (((EmptyPicker) picker).getStatus() == null) {
-        return "";
-      }
-      return ((EmptyPicker) picker).getStatus().toString();
-    } else if (picker instanceof ReadyPicker) {
       List<ChildLbState> 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);
       }
 
-    throw new IllegalArgumentException("Unrecognized picker: " + picker);
+      picker = childLbStates.get(0).getCurrentPicker();
+    }
+
+    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,
+    assertEquals(Status.OK, picker.pickSubchannel(mockArgs).getStatus());
-        picker.pickSubchannel(mockArgs).getStatus());
   }
 
   @Test
@@ -623,9 +608,8 @@ public class LeastRequestLoadBalancerTest {
 
   @Test
   public void internalPickerComparisons() {
-    EmptyPicker emptyOk1 = new EmptyPicker(Status.OK);
+    EmptyPicker empty1 = new EmptyPicker();
-    EmptyPicker emptyOk2 = new EmptyPicker(Status.OK.withDescription("different OK"));
+    EmptyPicker empty2 = new EmptyPicker();
-    EmptyPicker emptyErr = new EmptyPicker(Status.UNKNOWN.withDescription("¯\\_(ツ)_//¯"));
 
     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));
+    assertTrue(empty1.equals(empty2));
-    assertFalse(emptyOk1.isEquivalentTo(emptyErr));
+    assertFalse(ready1.equals(ready2));
-    assertFalse(ready1.isEquivalentTo(ready2));
+    assertTrue(ready1.equals(ready3));
-    assertTrue(ready1.isEquivalentTo(ready3));
+    assertTrue(ready3.equals(ready4));
-    assertTrue(ready3.isEquivalentTo(ready4));
+    assertTrue(ready4.equals(ready5));
-    assertTrue(ready4.isEquivalentTo(ready5));
+    assertFalse(empty1.equals(ready1));
-    assertFalse(emptyOk1.isEquivalentTo(ready1));
+    assertFalse(ready1.equals(empty1));
-    assertFalse(ready1.isEquivalentTo(emptyOk1));
+    assertFalse(ready5.equals(ready6));
-    assertFalse(ready5.isEquivalentTo(ready6));
   }
 
   @Test