loadbalancer-experimental: remove random-subsetting API from LoadBala… #3132
Conversation
…ncerBuilder Motivation: There are many forms of subsetting and I don't think we have sorted out the best way to make that configurable from an API perspective. As we make the LoadBalancerBuilder API non-experimental, we don't want to yet commit to the existing API. Modifications: - Remove the configuration from LoadBalancerBuilder - Extract the random subsetting logic into it's own abstraction to prep for future subsetting strategies Result: - Smaller API commitments - More flexibility to experiment internally
...loadbalancer-experimental/src/test/java/io/servicetalk/loadbalancer/RandomSubsetterTest.java
Show resolved
Hide resolved
...loadbalancer-experimental/src/test/java/io/servicetalk/loadbalancer/RandomSubsetterTest.java
Outdated
Show resolved
Hide resolved
| @@ -167,7 +156,7 @@ public LoadBalancer<C> newLoadBalancer( | |||
| new XdsOutlierDetector<>(executor, outlierDetectorConfig, lbDescription); | |||
| } | |||
| return new DefaultLoadBalancer<>(id, targetResource, eventPublisher, | |||
| DefaultHostPriorityStrategy::new, loadBalancingPolicy, subsetSize, | |||
| DefaultHostPriorityStrategy::new, loadBalancingPolicy, new RandomSubsetter(Integer.MAX_VALUE), | |||
There was a problem hiding this comment.
If we are not going to use a subsetter now, will it make sense to create a NoopSubsetter implementation that will simply return hosts as-is? It will be less code to execute inside LB if there is no need.
There was a problem hiding this comment.
We could, but it would be symbolic: RandomSubsetter checks if the desired subset is larger than the host set size and if so, it's a no-op. I'd like to keep that behavior even if we introduce a truly no-op subsetter.
There was a problem hiding this comment.
You are right, I think I misinterpreted the first if condition inside RandomSubsetter. All good.
| @@ -167,7 +156,7 @@ public LoadBalancer<C> newLoadBalancer( | |||
| new XdsOutlierDetector<>(executor, outlierDetectorConfig, lbDescription); | |||
| } | |||
| return new DefaultLoadBalancer<>(id, targetResource, eventPublisher, | |||
| DefaultHostPriorityStrategy::new, loadBalancingPolicy, subsetSize, | |||
| DefaultHostPriorityStrategy::new, loadBalancingPolicy, new RandomSubsetter(Integer.MAX_VALUE), | |||
There was a problem hiding this comment.
You are right, I think I misinterpreted the first if condition inside RandomSubsetter. All good.
Doh, you're right. Pushed now. |
bryce-anderson
force-pushed
the
bl_anderson/Subsetter
branch
from
36d99a3 to
7dfc0e3
Compare
…ncerBuilder
Motivation:
There are many forms of subsetting and I don't think we have sorted out the best way to make that configurable from an API perspective. As we make the LoadBalancerBuilder API non-experimental, we don't want to yet commit to the existing API.
Modifications:
Result: