loadbalancer: HostSelector can be rebuilt each time the DefaultLoadBalancer gets a host set update #2774

bryce-anderson · 2023-12-07T20:07:20Z

Motivation:

For algorithms like weighted round robin and random we need to
build some data structures for each unique host set.

Modifications:

Add a .rebuild(List<Host>) method to the HostSelector
interface. This lets a host potentially rebuild data structures it
needs to properly balance but only when there is a host set update.
Add a .isHealthy() method to HostSelector. This cleaned up
some questions about which host list is used and may also be
helpful for future composition of selectors.
Cleanup some thread safety considerations in DefaultLoadBalancer.

Motivation: For algorithms like weighted round robin and weighted random we need to build some data structures for each unique host set. Modifications: Add a `.rebuild(List<Host>)` method to the `HostSelector` interface. This lets a host potentially rebuild data structures it needs to properly balance but only when there is a host set update.

bryce-anderson · 2023-12-07T22:39:36Z

...icetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/LoadBalancerTestScaffold.java

+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+abstract class LoadBalancerTestScaffold {


This is all pulled from LoadBalancerTest.

bryce-anderson · 2023-12-07T22:39:59Z

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

@@ -127,33 +111,12 @@
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;

-abstract class LoadBalancerTest {
+abstract class LoadBalancerTest extends LoadBalancerTestScaffold {


All the deletions here were simply moved to LoadBalancerTestScaffold.

bryce-anderson · 2023-12-08T00:01:54Z

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


    SequentialExecutor(final ExceptionHandler exceptionHandler) {
        this.exceptionHandler = requireNonNull(exceptionHandler, "exceptionHandler");
    }

+    public boolean isCurrentThreadDraining() {


This is only really necessary so we can make DefaultLoadBalancer.usedHosts non-volatile and still have a thread safe DefaultLoadBalancer.usedAddresses() method, but it feels 'nice'. Feedback welcome on it's utility.
Also see the comment in usedAddresses().

bryce-anderson · 2023-12-08T00:03:50Z

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

+        // If we're already in the executor we can't submit a task and wait for it without deadlock but
+        // the access is thread safe anyway so just go for it.
+        if (sequentialExecutor.isCurrentThreadDraining()) {
+            return sequentialUsedAddresses();
+        }
+        CompletableFuture<List<Entry<ResolvedAddress, List<C>>>> future = new CompletableFuture<>();
+        sequentialExecutor.execute(() -> future.complete(sequentialUsedAddresses()));
+        try {
+            // This method is just for testing, so it's fine to do some awaiting.
+            return future.get(5, TimeUnit.SECONDS);
+        } catch (Exception ex) {
+            throw new AssertionError("Failed to get results", ex);
+        }


If we wanted to, this pattern could be lifted into a method of the form T SequentialExecutor.submit(Callable<T>).

idelpivnitskiy

LGTM, please get a 2nd eye on SequentialExecutor and removal of volatile from DefaultLoadBalancer

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

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

…elector

tkountis · 2023-12-08T23:41:27Z

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

@@ -502,6 +509,24 @@ public Completable closeAsyncGracefully() {

    @Override
    public List<Entry<ResolvedAddress, List<C>>> usedAddresses() {


I may be missing something here, but isn't that a check-then-act issue? What guarantees that the condition still holds after the IF.

Can you clarify what your concern is? If it's whether the result from usedAddresses() is still valid, then yes: it's always going to be racy. Since it's only used for tests to inspect the internal state the usage is rather constrained it should be no worse than it always was. If you means the if block on line 514 then we know what the current sequence of calls are going to be for this thread and reason about it for the lifetime of this method call at least.

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

daschl · 2023-12-11T08:41:02Z

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

+ * pattern to use is to make the internal state effectively immutable and rebuilds (see below) generate new
+ * immutable instances as necessary. The interface is expected to adhere to the following rules:
+ *
+ * <li>The {@link HostSelector#selectConnection(Predicate, ContextMap, boolean)} method will be used


So to clarify: the old host selector could still be called even if a new rebuilt host selector has been returned?

That is not strictly specified. It's possible to envision a use case where the old-selector gets 'locked down' when a new selector is built. However, the common case (and perhaps the only real case) should be that sending a request to an old selector will be treated as if it had simply raced ahead of the rebuild.

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

bryce-anderson requested review from daschl, mgodave, tkountis, idelpivnitskiy and Scottmitch December 7, 2023 20:07

bryce-anderson mentioned this pull request Dec 7, 2023

loadbalancer: Change generics of LoadBalancerPolicy #2773

Merged

bryce-anderson force-pushed the bl_anderson/rebuild-selector branch from 546ed33 to ebb41e4 Compare December 7, 2023 21:56

Some lifecycle management changes

b7365af

bryce-anderson commented Dec 7, 2023

View reviewed changes

bryce-anderson added 2 commits December 7, 2023 16:30

Cleanup the concurrrency model in DefaultLoadBalancer a bit more

6ed40e2

More thread safety stuff

cc412c6

bryce-anderson force-pushed the bl_anderson/rebuild-selector branch from d951ba1 to cc412c6 Compare December 8, 2023 00:00

bryce-anderson commented Dec 8, 2023

View reviewed changes

bryce-anderson marked this pull request as ready for review December 8, 2023 00:06

bryce-anderson changed the title ~~loadbalancer: HostSelector can be rebuilt~~ loadbalancer: HostSelector can be rebuilt each time the DefaultLoadBalancer gets a host set update Dec 8, 2023

bryce-anderson mentioned this pull request Dec 8, 2023

loadbalancer: Use a sequential execution concurrency model in DefaultLoadBalancer #2768

Merged

idelpivnitskiy approved these changes Dec 8, 2023

View reviewed changes

bryce-anderson added 2 commits December 8, 2023 11:07

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

e58b44e

…elector

Idel feedback

edb4b0e

tkountis approved these changes Dec 8, 2023

View reviewed changes

daschl reviewed Dec 11, 2023

View reviewed changes

feedback

afabb6d

bryce-anderson requested a review from daschl December 11, 2023 16:58

bryce-anderson merged commit bad9558 into apple:main Dec 11, 2023
15 checks passed

bryce-anderson deleted the bl_anderson/rebuild-selector branch December 11, 2023 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loadbalancer: HostSelector can be rebuilt each time the DefaultLoadBalancer gets a host set update #2774

loadbalancer: HostSelector can be rebuilt each time the DefaultLoadBalancer gets a host set update #2774

bryce-anderson commented Dec 7, 2023 •

edited

bryce-anderson Dec 7, 2023

bryce-anderson Dec 7, 2023

bryce-anderson Dec 8, 2023 •

edited

bryce-anderson Dec 8, 2023

idelpivnitskiy left a comment

tkountis Dec 8, 2023

bryce-anderson Dec 11, 2023 •

edited

daschl Dec 11, 2023

bryce-anderson Dec 11, 2023

		@@ -502,6 +509,24 @@ public Completable closeAsyncGracefully() {

		@Override
		public List<Entry<ResolvedAddress, List<C>>> usedAddresses() {

loadbalancer: HostSelector can be rebuilt each time the DefaultLoadBalancer gets a host set update #2774

loadbalancer: HostSelector can be rebuilt each time the DefaultLoadBalancer gets a host set update #2774

Conversation

bryce-anderson commented Dec 7, 2023 • edited

bryce-anderson Dec 7, 2023

Choose a reason for hiding this comment

bryce-anderson Dec 7, 2023

Choose a reason for hiding this comment

bryce-anderson Dec 8, 2023 • edited

Choose a reason for hiding this comment

bryce-anderson Dec 8, 2023

Choose a reason for hiding this comment

idelpivnitskiy left a comment

Choose a reason for hiding this comment

tkountis Dec 8, 2023

Choose a reason for hiding this comment

bryce-anderson Dec 11, 2023 • edited

Choose a reason for hiding this comment

daschl Dec 11, 2023

Choose a reason for hiding this comment

bryce-anderson Dec 11, 2023

Choose a reason for hiding this comment

bryce-anderson commented Dec 7, 2023 •

edited

bryce-anderson Dec 8, 2023 •

edited

bryce-anderson Dec 11, 2023 •

edited