loadbalancer: add builder for the DefaultLoadBalancer #2749

bryce-anderson · 2023-11-09T22:09:01Z

Motivation:

We have a P2C selector but no easy way to use it.

Modifications:

add a package private (for now) builder pattern for the DefaultLoadBalancer
refactor the testing so that the DefaultLoadBalancer can use the existing unit tests

idelpivnitskiy · 2023-11-12T00:46:31Z

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicy.java

+/**
+ * Definition of the selector mechanism used for load balancing.
+ */
+public interface LoadBalancingPolicy {


+1 for going forward with this approach, this is more aligned with other use-cases, like configuring different HTTP protocol versions on client/server builder and makes it easier to manage externally via ServiceLoader providers if there is only one LB builder with only one provider.

Not now, but in the future we may need to add a "selector" as a method on LoadBalancingPolicy to let users implement their own selectors. For now, +1 for keeping that detail pkg-private.

Scottmitch · 2023-11-14T14:45:18Z

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilder.java

+
+import java.time.Duration;
+
+public interface LoadBalancerBuilder<ResolvedAddress, C extends LoadBalancedConnection> {


do we need an interface for a builder, vs a concrete class? Will we need this interface internally? Often times concrete builders may have different ways to configure themselves and may not needs the same options.

Yes, interface is helpful when we add support for ServiceLoader providers

@idelpivnitskiy, couldn't we do something like the RoundRobinLoadBalancers strategy? We could make it a concrete class, make the constructor private, then funneling all builder(String id)'s through the providers that get first dibs to set defaults.

I pushed a commit that seems along the lines of what @Scottmitch is suggesting. I like it.

It's a simplistic approach that we use for cases, when we don't need a ServiceLoader provider, like RedirectConfigBuilder, ProxyConfigBuilder, H1ProtocolConfigBuilder, etc. For major objects, like client/server builder, DnsServiceDiscovererBuilder, LB builder it's extremely useful to have access through providers. Providers require them to be interfaces bcz users need to be able to wrap a builder. Wrapper monitors what users set on the builder explicitly and at the build time can modify or add if users missed something. See simple examples in HttpProvidersTest.

tkountis · 2023-11-16T01:39:35Z

+1 for this approach

We discussed offline, just to keep everything in one place:

Unrelated to this PR, but i would love an alternative way of applying the outlier detection features. ie. a form of typed-filters (l4-filter, l7-filter etc). -> the builder becomes agnostic of the individual feature
+1 on @idelpivnitskiy point about having support for a custom selector. We can't support all possible features of a LB out of the box, so it would be nice if users had the ability to modify as needed e.g. adding sticky-sessions.

…ers in place

bryce-anderson · 2023-11-18T00:48:18Z

Thanks for the feedback everyone. This seems like the path we want to take so I'm rounding it out now. It's also going to be pretty big so I'm pulling smaller chunks out as smaller PR's for easier reviewing.

bryce-anderson · 2023-11-28T22:37:48Z

...dbalancer/src/test/java/io/servicetalk/loadbalancer/LingeringRoundRobinLoadBalancerTest.java

-    @Test
-    void hostDownDoesntCloseConnectionCloseLB() throws Exception {
-        hostDownDoesntCloseConnection(false);
-    }
-
-    @Test
-    void hostDownDoesntCloseConnectionCloseLBGracefully() throws Exception {
-        hostDownDoesntCloseConnection(true);
-    }


The tests were moved to LingeringLoadBalancerTest.

bryce-anderson · 2023-11-28T22:38:21Z

...-loadbalancer/src/test/java/io/servicetalk/loadbalancer/EagerRoundRobinLoadBalancerTest.java

-    @Test
-    void duplicateEventsAreIgnored() {
-        assertThat(lb.usedAddresses(), is(empty()));
-
-        sendServiceDiscoveryEvents(upEvent("address-1"));
-        assertThat(lb.usedAddresses(), hasSize(1));
-        sendServiceDiscoveryEvents(upEvent("address-1"));
-        assertThat(lb.usedAddresses(), hasSize(1));
-
-        sendServiceDiscoveryEvents(downEvent("address-1"));
-        assertThat(lb.usedAddresses(), hasSize(0));
-        sendServiceDiscoveryEvents(downEvent("address-1"));
-        assertThat(lb.usedAddresses(), hasSize(0));
-    }
-
-    @Test
-    void handleDiscoveryEvents() {


The tests were all moved to EagerLoadBalancerTest so they can be shared.

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicies.java

bryce-anderson · 2023-11-29T21:12:09Z

servicetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/EagerLoadBalancerTest.java

+abstract class EagerLoadBalancerTest extends LoadBalancerTest {
+
+    @Test
+    void duplicateEventsAreIgnored() {


These tests were moved from EagerRoundRobinLoadBalancerTest.java to here.

bryce-anderson · 2023-11-29T21:13:32Z

...cetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/LingeringLoadBalancerTest.java

+abstract class LingeringLoadBalancerTest extends LoadBalancerTest {
+
+    @Test
+    void hostDownDoesntCloseConnectionCloseLB() throws Exception {


These tests were moved from LingeringRoundRobinLoadBalancerTest.java.

tkountis

lgtm

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/P2CLoadBalancingPolicy.java

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilder.java

…_option2

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilder.java

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicy.java

...lk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancingPolicy.java

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicies.java

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/P2CLoadBalancingPolicy.java

...talk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilderProvider.java

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilder.java

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicy.java

WIP of using a sub-structure for configuring the LB policy

95991ad

bryce-anderson requested review from mgodave, tkountis and idelpivnitskiy November 9, 2023 22:09

bryce-anderson mentioned this pull request Nov 9, 2023

RFC: Option 1: config different LB selections policies by inheritance from a base builder #2750

Closed

idelpivnitskiy reviewed Nov 12, 2023

View reviewed changes

Scottmitch reviewed Nov 14, 2023

View reviewed changes

bryce-anderson added 2 commits November 15, 2023 17:03

This may be closer to what Scott wanted, and still lets us do providers

feaba42

See what adding a selector specific call could look like

7274e13

bryce-anderson added 6 commits November 17, 2023 12:24

Merge branch 'main' into bl_anderson/lb_config_option2

57f47f1

Back to an interface for Provider purposes

d97123f

Rename NewRoundRobinLoadBalancer to DefaultLoadBalancer and get build…

3776296

…ers in place

Finish some builds and reshape some tests

e904972

Add P2C tests

2030c86

Fix style problems

35fbf21

This was referenced Nov 18, 2023

loadbalancer: rename NewRoundRobinLoadBalancer to DefaultLoadBalancer #2763

Merged

loadbalancer: move health check related constants to the HealthCheck class #2764

Merged

bryce-anderson added 2 commits November 27, 2023 09:39

Merge branch 'main' into bl_anderson/lb_config_option2

4fda3c2

Some tidy ups

3c3a9e4

bryce-anderson changed the title ~~RFC: Option 2: pass in specialized LB policy configs~~ Add configuration builders for the DefaultLoadBalancer Nov 28, 2023

bryce-anderson changed the title ~~Add configuration builders for the DefaultLoadBalancer~~ loadbalancer: add builder for the DefaultLoadBalancer Nov 28, 2023

bryce-anderson commented Nov 28, 2023

View reviewed changes

bryce-anderson added 4 commits November 28, 2023 16:07

Add some javadocs

3b0a608

Fix up some tests

cd4a636

Make tests work with p2c

f4b5887

More docs

e818152

bryce-anderson commented Nov 29, 2023

View reviewed changes

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicies.java Outdated Show resolved Hide resolved

bryce-anderson commented Nov 29, 2023

View reviewed changes

Add maxEffort to the P2C name

2200404

bryce-anderson marked this pull request as ready for review November 29, 2023 21:24

bryce-anderson requested review from Scottmitch and idelpivnitskiy November 29, 2023 21:24

tkountis approved these changes Dec 4, 2023

View reviewed changes

bryce-anderson added 3 commits December 4, 2023 15:37

Thomas feedback

5393866

Merge remote-tracking branch 'origin/main' into bl_anderson/lb_config…

cca43e6

…_option2

Fixes for new pmd rules

b3b0f42

bryce-anderson requested a review from daschl December 5, 2023 17:50

idelpivnitskiy reviewed Dec 5, 2023

View reviewed changes

bryce-anderson added 2 commits December 5, 2023 16:41

Idel feedback

1beec8c

Make static instances for the both p2c and rr default lb policies

3e393f6

bryce-anderson requested a review from idelpivnitskiy December 6, 2023 00:29

idelpivnitskiy approved these changes Dec 6, 2023

View reviewed changes

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilder.java Show resolved Hide resolved

servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicy.java Show resolved Hide resolved

Another javadoc

b27acb7

bryce-anderson merged commit ad80596 into apple:main Dec 6, 2023
15 checks passed

bryce-anderson deleted the bl_anderson/lb_config_option2 branch December 6, 2023 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loadbalancer: add builder for the DefaultLoadBalancer #2749

loadbalancer: add builder for the DefaultLoadBalancer #2749

bryce-anderson commented Nov 9, 2023 •

edited

idelpivnitskiy Nov 12, 2023

Scottmitch Nov 14, 2023

idelpivnitskiy Nov 15, 2023

bryce-anderson Nov 15, 2023 •

edited

bryce-anderson Nov 16, 2023

idelpivnitskiy Nov 16, 2023

tkountis commented Nov 16, 2023 •

edited

bryce-anderson commented Nov 18, 2023

bryce-anderson Nov 28, 2023

bryce-anderson Nov 28, 2023

bryce-anderson Nov 29, 2023

bryce-anderson Nov 29, 2023

tkountis left a comment


		import java.time.Duration;

		public interface LoadBalancerBuilder<ResolvedAddress, C extends LoadBalancedConnection> {

loadbalancer: add builder for the DefaultLoadBalancer #2749

loadbalancer: add builder for the DefaultLoadBalancer #2749

Conversation

bryce-anderson commented Nov 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryce-anderson Nov 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkountis commented Nov 16, 2023 • edited

bryce-anderson commented Nov 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkountis left a comment

Choose a reason for hiding this comment

bryce-anderson commented Nov 9, 2023 •

edited

bryce-anderson Nov 15, 2023 •

edited

tkountis commented Nov 16, 2023 •

edited