loadbalancer: add an observer pattern to DefaultLoadBalancer #2770
Conversation
bryce-anderson
requested review from
tkountis,
idelpivnitskiy,
Scottmitch and
mgodave
| import java.util.function.Function; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| class TestConnectionFactory implements |
There was a problem hiding this comment.
This was extracted from LoadBalancerTest.
| import static io.servicetalk.concurrent.api.Single.failed; | ||
| import static java.util.concurrent.TimeUnit.SECONDS; | ||
|
|
||
| class UnhealthyHostConnectionFactory { |
There was a problem hiding this comment.
This was extracted from LoadBalancerTest.
| // TODO: do we need to signal closed for this host? Why isn't it closed? The | ||
| // closing should probably be re-worked to funnel closing behavior through one place | ||
| // and also define what being closed means: just the host isn't used anymore for new | ||
| // requests/connections or does it also mean that all connections have closed? |
There was a problem hiding this comment.
This looks like a bug but not one that I want to address here. I think there will be a bit more massaging in DefaultHost now that we have SequentialExecutor and can probably extract the outlier detector and let it synchronously read host state so long as it's within the executor.
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultHost.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultHost.java
Outdated
Show resolved
Hide resolved
| * An observer that provides visibility into a {@link io.servicetalk.client.api.LoadBalancer} | ||
| * @param <ResolvedAddress> the type of the resolved address. | ||
| */ | ||
| interface LoadBalancerObserver<ResolvedAddress> { |
There was a problem hiding this comment.
I'd prefer to keep it private until we are a bit more confident, although that is kind of a chicken and egg problem. Can we tag it with the @UnstableApi annotation or something or is that a no-go?
There was a problem hiding this comment.
The typical way so-far was the experimental package. We can have such a package and move it out when we are ready.
| */ | ||
| void noActiveHostsAvailable(int hostSetSize, NoActiveHostException exn); | ||
|
|
||
| interface HostObserver<ResolvedAddress> { |
There was a problem hiding this comment.
Should we also define selectivity factors here, regardless if the current implementation uses them?
There was a problem hiding this comment.
Can you elaborate? I think you're considering something like "score changed" or something like that but I want to be sure. If that is the case, I'd slightly prefer to just add them when we have an example since.
There was a problem hiding this comment.
Correct, i was thinking something like selectionOddsChanged(host, odds)
Defining odds in a meaningful consumable form, sounds interesting. Example, we could introduce rational groups (low, normal, high) and use them to categorize the hosts.
Happy to do in a later revision.
There was a problem hiding this comment.
Yes, that will be interesting but let's add it later when we have data to feed it.
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerObserver.java
Outdated
Show resolved
Hide resolved
Motivation:
With a more interesting load balancer we're going to want
more visibility into what is happening.
Modifications:
Add an observer interface to thread to the load balancer.
This interface has a sub-interface for host concerns and we
can add more sub-interfaces as we continue to break parts
down.