From 1aafb7a04458c6aa9d8f1c6bf1caa6b8953611ad Mon Sep 17 00:00:00 2001
From: Bryce Anderson <bryce_anderson@apple.com>
Date: Thu, 28 Mar 2024 09:08:53 -0600
Subject: [PATCH] loadbalancer-experimental: tighten up load balancing policy
 (#2884)

Motivation:

The LoadBalancingPolicy interface suffers from the same problems
that the OutlierDetectorFactory did, namely that it's more powerful
than users need.

Modifications:

- Make the LoadBalancingPolicy an abstract class with package private
  constructor to let us control its proliferation for now.

Result:

A more constrained API. We can always open it up more later if we
want to.
---
 .../loadbalancer/LoadBalancingPolicy.java         | 15 +++++++++++----
 .../loadbalancer/P2CLoadBalancingPolicy.java      |  6 +++---
 .../RoundRobinLoadBalancingPolicy.java            |  9 +++++++--
 .../loadbalancer/DefaultLoadBalancerTest.java     | 11 ++++++++---
 4 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicy.java b/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicy.java
index d26f286b34..3308158e31 100644
--- a/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicy.java
+++ b/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicy.java
@@ -24,18 +24,25 @@
  * @param <ResolvedAddress> the type of the resolved address
  * @param <C> the type of the load balanced connection
  */
-public interface LoadBalancingPolicy<ResolvedAddress, C extends LoadBalancedConnection> {
+public abstract class LoadBalancingPolicy<ResolvedAddress, C extends LoadBalancedConnection> {
 
     /**
      * The default fail-open policy to use for {@link HostSelector} implementations.
      */
-    boolean DEFAULT_FAIL_OPEN_POLICY = false;
+    static final boolean DEFAULT_FAIL_OPEN_POLICY = false;
+
+    LoadBalancingPolicy() {
+        // package private constructor to control proliferation.
+    }
 
     /**
      * The name of the load balancing policy.
      * @return the name of the load balancing policy
      */
-    String name();
+    public abstract String name();
+
+    @Override
+    public abstract String toString();
 
     /**
      * Construct a {@link HostSelector}.
@@ -43,6 +50,6 @@ public interface LoadBalancingPolicy<ResolvedAddress, C extends LoadBalancedConn
      * @param targetResource the name of the target resource, useful for debugging purposes.
      * @return a {@link HostSelector}
      */
-    HostSelector<ResolvedAddress, C> buildSelector(
+    abstract HostSelector<ResolvedAddress, C> buildSelector(
             List<Host<ResolvedAddress, C>> hosts, String targetResource);
 }
diff --git a/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/P2CLoadBalancingPolicy.java b/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/P2CLoadBalancingPolicy.java
index ce77f62258..177c91ca56 100644
--- a/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/P2CLoadBalancingPolicy.java
+++ b/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/P2CLoadBalancingPolicy.java
@@ -40,7 +40,7 @@
  *  *  Choices in Randomized Load Balancing</a>
  */
 public final class P2CLoadBalancingPolicy<ResolvedAddress, C extends LoadBalancedConnection>
-        implements LoadBalancingPolicy<ResolvedAddress, C> {
+        extends LoadBalancingPolicy<ResolvedAddress, C> {
 
     private final int maxEffort;
     private final boolean failOpen;
@@ -54,7 +54,7 @@ private P2CLoadBalancingPolicy(final int maxEffort, final boolean failOpen, @Nul
     }
 
     @Override
-    public HostSelector<ResolvedAddress, C> buildSelector(
+    HostSelector<ResolvedAddress, C> buildSelector(
             List<Host<ResolvedAddress, C>> hosts, String targetResource) {
         return new P2CSelector<>(hosts, targetResource, maxEffort, failOpen, random);
     }
@@ -66,7 +66,7 @@ public String name() {
 
     @Override
     public String toString() {
-        return name() + "(maxEffort=" + maxEffort + ')';
+        return name() + "(failOpen=" + failOpen + ", maxEffort=" + maxEffort + ')';
     }
 
     /**
diff --git a/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancingPolicy.java b/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancingPolicy.java
index dfd13a3338..9146986463 100644
--- a/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancingPolicy.java
+++ b/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancingPolicy.java
@@ -28,7 +28,7 @@
  * @param <C> the type of the load balanced connection
  */
 public final class RoundRobinLoadBalancingPolicy<ResolvedAddress, C extends LoadBalancedConnection>
-        implements LoadBalancingPolicy<ResolvedAddress, C> {
+        extends LoadBalancingPolicy<ResolvedAddress, C> {
 
     private final boolean failOpen;
 
@@ -37,7 +37,7 @@ private RoundRobinLoadBalancingPolicy(final boolean failOpen) {
     }
 
     @Override
-    public HostSelector<ResolvedAddress, C>
+    HostSelector<ResolvedAddress, C>
     buildSelector(final List<Host<ResolvedAddress, C>> hosts, final String targetResource) {
         return new RoundRobinSelector<>(hosts, targetResource, failOpen);
     }
@@ -47,6 +47,11 @@ public String name() {
         return "RoundRobin";
     }
 
+    @Override
+    public String toString() {
+        return name() + "(failOpen=" + failOpen + ")";
+    }
+
     /**
      * A builder for immutable {@link RoundRobinLoadBalancingPolicy} instances.
      */
diff --git a/servicetalk-loadbalancer-experimental/src/test/java/io/servicetalk/loadbalancer/DefaultLoadBalancerTest.java b/servicetalk-loadbalancer-experimental/src/test/java/io/servicetalk/loadbalancer/DefaultLoadBalancerTest.java
index 9209718e3c..c628b8abd0 100644
--- a/servicetalk-loadbalancer-experimental/src/test/java/io/servicetalk/loadbalancer/DefaultLoadBalancerTest.java
+++ b/servicetalk-loadbalancer-experimental/src/test/java/io/servicetalk/loadbalancer/DefaultLoadBalancerTest.java
@@ -294,17 +294,22 @@ List<TestHealthIndicator> getIndicators() {
         }
     }
 
-    private static class TestLoadBalancerPolicy implements LoadBalancingPolicy<String, TestLoadBalancedConnection> {
+    private static class TestLoadBalancerPolicy extends LoadBalancingPolicy<String, TestLoadBalancedConnection> {
 
         int rebuilds;
 
         @Override
         public String name() {
-            return "test-selector";
+            return "TestPolicy";
         }
 
         @Override
-        public HostSelector<String, TestLoadBalancedConnection> buildSelector(
+        public String toString() {
+            return name() + "()";
+        }
+
+        @Override
+        HostSelector<String, TestLoadBalancedConnection> buildSelector(
                 List<Host<String, TestLoadBalancedConnection>> hosts, String targetResource) {
             return new TestSelector(hosts);
         }