loadbalancer-experimental: HostObserver shouldn't need a type param #2821
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: The current HostObserver API requires one to pass in the ResolvedAddress on each callback. This means that every place we want to observer events also needs to know the address which can be a pain. Modifications: - Drop the type parameter from HostObserver - Make LoadBalancerObserver take the address as a parameter of the `hostObserver(..)` method. - Propagate those changes all over the place Result: What is the result of this change?
| * @param cause the most recent cause of the transition. | ||
| */ | ||
| void onHostMarkedUnhealthy(ResolvedAddress address, @Nullable Throwable cause); | ||
| void onHostMarkedUnhealthy(@Nullable Throwable cause); |
There was a problem hiding this comment.
Seems like we always have the cause, should we remove @Nullable annotation?
| @@ -135,13 +135,13 @@ public LoadBalancerFactory<ResolvedAddress, C> build() { | |||
| } | |||
| final LoadBalancerObserver<ResolvedAddress> loadBalancerObserver = this.loadBalancerObserver != null ? | |||
| this.loadBalancerObserver : NoopLoadBalancerObserver.instance(); | |||
| Supplier<HealthChecker<ResolvedAddress>> healthCheckerSupplier; | |||
| Function<String, HealthChecker<ResolvedAddress>> healthCheckerSupplier; | |||
There was a problem hiding this comment.
healthCheckerSupplier -> healthCheckerFactory?
Also, consider marking it as final
| @Nullable | ||
| private final HealthCheckConfig healthCheckConfig; | ||
|
|
||
| DefaultLoadBalancerFactory(final String id, final LoadBalancingPolicy<ResolvedAddress, C> loadBalancingPolicy, | ||
| final int linearSearchSpace, final HealthCheckConfig healthCheckConfig, | ||
| final LoadBalancerObserver<ResolvedAddress> loadBalancerObserver, | ||
| final Supplier<HealthChecker<ResolvedAddress>> healthCheckerFactory) { | ||
| final Function<String, HealthChecker<ResolvedAddress>> healthCheckerFactory) { |
There was a problem hiding this comment.
This arg should have @Nullable annotation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
The current HostObserver API requires one to pass in the ResolvedAddress on each callback. This means that every place we want to observer events also needs to know the address which can be a pain.
Modifications:
HostObserverLoadBalancerObservertake the address as a parameter of thehostObserver(..)methodHealthCheckerinterfaces to take theHostObserverduring construction of the associatedHealthIndicator