loadbalancer: introduce the HealthChecker interface #2800
Merged
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: We want to make health checking more module in DefaultLoadBalancer. Modifications: - Add the HealthChecker interface and the supporting cast. - Integrate the HealthChecker into DefaultLoadBalancer. - Adjust the LatencyTracker interface and extend it to support HealthChecker. Result: A modular way to support health checking.
bryce-anderson
requested review from
mgodave,
tkountis,
idelpivnitskiy and
Scottmitch
tkountis
approved these changes
Jan 12, 2024
There was a problem hiding this comment.
On purpose? Is there a replacement that i missed?
There was a problem hiding this comment.
I did delete it on purpose. I suppose we can keep it, but I had hoped that we could use the class by extension instead of by proxy so that we can avoid the questions of which delegate to pick. That is important because if they have different time sources we can have really weird bugs.
There was a problem hiding this comment.
I brought it back just in case we want it later. I'm still of the opinion that we need to somehow enforce the same time source for both delegates but we can continue to iterate on it.
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthCheckerFactory.java
Outdated
Show resolved
Hide resolved
mgodave
approved these changes
Jan 12, 2024
bryce-anderson
deleted the
bl_anderson/L7OutlierDetector_pt1-interfaces
branch
| @@ -52,16 +53,4 @@ interface LatencyTracker extends ScoreSupplier { | |||
| * @param beforeStartTimeNs return value from {@link #beforeStart()}. | |||
| */ | |||
| void observeError(long beforeStartTimeNs); | |||
There was a problem hiding this comment.
Optional: consider onSuccess/onError/onCancel instead of observe* to be consistent with other places
| /** | ||
| * The representation of a health checking system for use with load balancing. | ||
| * <p> | ||
| * The core |
There was a problem hiding this comment.
should it have continuation?
| * The current time in nanoseconds. | ||
| * @return the current time in nanoseconds. | ||
| */ | ||
| protected abstract long currentTimeNanos(); |
There was a problem hiding this comment.
If the DefaultRequestTracker can access Executor from HealthCheckerFactory.newHealthChecker(...), I would recommend using Executor as a TimeSource instead of adding an internal hook here, as a consistent approach we use in other places.
| if (context == null) { | ||
| context = new DefaultContextMap(); | ||
| } | ||
| context.put(REQUEST_TRACKER_KEY, healthIndicator); |
There was a problem hiding this comment.
It's not ideal that we have to rely on context. Let's see how we retrieve it from the context in follow-up PRs, maybe there will be a better way.
There was a problem hiding this comment.
I'm open to suggestions. This is The best both Thomas and I have thought of so far playing within the constraints of ConnectionFactory and the *Requester interfaces.
| @Nullable final HealthCheckConfig healthCheckConfig, | ||
| @Nullable final LoadBalancerObserver<ResolvedAddress> loadBalancerObserver) { | ||
| @Nullable final Supplier<HealthChecker> healthCheckerFactory) { |
There was a problem hiding this comment.
Supplier<HealthChecker> -> Supplier<HealthChecker<ResolvedAddress>>, we should also add parameters for LoadBalancingPolicy
| this.sequentialExecutor = new SequentialExecutor((uncaughtException) -> | ||
| LOGGER.error("{}: Uncaught exception in SequentialExecutor", this, uncaughtException)); | ||
| LOGGER.error("{}: Uncaught exception in " + this.getClass().getSimpleName(), this, uncaughtException)); |
There was a problem hiding this comment.
Consider using logger formatting instead of concatenation
| @@ -0,0 +1,36 @@ | |||
| /* | |||
| * Copyright © 2023 Apple Inc. and the ServiceTalk project authors | |||
There was a problem hiding this comment.
Some of my comments addressed in this patch:
Subject: [PATCH] comments
---
Index: servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilder.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilder.java b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilder.java
--- a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilder.java (revision 34db3ac6fde1fd333efc146b37b70506642e51f0)
+++ b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilder.java (date 1705108853059)
@@ -89,7 +89,8 @@
* {@link HealthChecker}.
* @return {code this}
*/
- LoadBalancerBuilder<ResolvedAddress, C> healthCheckerFactory(HealthCheckerFactory healthCheckerFactory);
+ LoadBalancerBuilder<ResolvedAddress, C> healthCheckerFactory(
+ HealthCheckerFactory<ResolvedAddress> healthCheckerFactory);
/**
* This {@link LoadBalancer} may monitor hosts to which connection establishment has failed
Index: servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java
--- a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java (revision 34db3ac6fde1fd333efc146b37b70506642e51f0)
+++ b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java (date 1705108572725)
@@ -133,7 +133,7 @@
final int linearSearchSpace,
final LoadBalancerObserver<ResolvedAddress> loadBalancerObserver,
@Nullable final HealthCheckConfig healthCheckConfig,
- @Nullable final Supplier<HealthChecker> healthCheckerFactory) {
+ @Nullable final Supplier<HealthChecker<ResolvedAddress>> healthCheckerFactory) {
this.targetResource = requireNonNull(targetResourceName);
this.lbDescription = makeDescription(id, targetResource);
this.hostSelector = requireNonNull(hostSelector, "hostSelector");
Index: servicetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancerBuilderAdapter.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancerBuilderAdapter.java b/servicetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancerBuilderAdapter.java
--- a/servicetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancerBuilderAdapter.java (revision 34db3ac6fde1fd333efc146b37b70506642e51f0)
+++ b/servicetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancerBuilderAdapter.java (date 1705108853063)
@@ -44,7 +44,7 @@
@Override
public LoadBalancerBuilder<String, TestLoadBalancedConnection> healthCheckerFactory(
- HealthCheckerFactory healthCheckerFactory) {
+ HealthCheckerFactory<String> healthCheckerFactory) {
throw new IllegalStateException("Cannot set a load balancer health checker for old round robin");
}
Index: servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthChecker.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthChecker.java b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthChecker.java
--- a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthChecker.java (revision 34db3ac6fde1fd333efc146b37b70506642e51f0)
+++ b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthChecker.java (date 1705107787951)
@@ -1,5 +1,5 @@
/*
- * Copyright © 2023 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2024 Apple Inc. and the ServiceTalk project authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Index: servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthCheckerFactory.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthCheckerFactory.java b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthCheckerFactory.java
--- a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthCheckerFactory.java (revision 34db3ac6fde1fd333efc146b37b70506642e51f0)
+++ b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthCheckerFactory.java (date 1705107831292)
@@ -1,5 +1,5 @@
/*
- * Copyright © 2023 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2024 Apple Inc. and the ServiceTalk project authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Index: servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RequestTracker.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RequestTracker.java b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RequestTracker.java
--- a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RequestTracker.java (revision 34db3ac6fde1fd333efc146b37b70506642e51f0)
+++ b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RequestTracker.java (date 1705107576296)
@@ -1,5 +1,5 @@
/*
- * Copyright © 2020 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2023-2024 Apple Inc. and the ServiceTalk project authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -22,7 +22,6 @@
*/
interface RequestTracker {
- @SuppressWarnings("rawtypes")
ContextMap.Key<RequestTracker> REQUEST_TRACKER_KEY =
ContextMap.Key.newKey("request_tracker", RequestTracker.class);
Index: servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancerBuilder.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancerBuilder.java b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancerBuilder.java
--- a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancerBuilder.java (revision 34db3ac6fde1fd333efc146b37b70506642e51f0)
+++ b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancerBuilder.java (date 1705108853061)
@@ -51,7 +51,7 @@
@Nullable
private LoadBalancerObserver<ResolvedAddress> loadBalancerObserver;
@Nullable
- private HealthCheckerFactory healthCheckerFactory;
+ private HealthCheckerFactory<ResolvedAddress> healthCheckerFactory;
private Duration healthCheckInterval = DEFAULT_HEALTH_CHECK_INTERVAL;
private Duration healthCheckJitter = DEFAULT_HEALTH_CHECK_JITTER;
private int healthCheckFailedConnectionsThreshold = DEFAULT_HEALTH_CHECK_FAILED_CONNECTIONS_THRESHOLD;
@@ -83,7 +83,8 @@
}
@Override
- public LoadBalancerBuilder<ResolvedAddress, C> healthCheckerFactory(HealthCheckerFactory healthCheckerFactory) {
+ public LoadBalancerBuilder<ResolvedAddress, C> healthCheckerFactory(
+ HealthCheckerFactory<ResolvedAddress> healthCheckerFactory) {
this.healthCheckerFactory = healthCheckerFactory;
return this;
}
@@ -133,7 +134,7 @@
}
final LoadBalancerObserver<ResolvedAddress> loadBalancerObserver = this.loadBalancerObserver != null ?
this.loadBalancerObserver : NoopLoadBalancerObserver.instance();
- Supplier<HealthChecker> healthCheckerSupplier;
+ Supplier<HealthChecker<ResolvedAddress>> healthCheckerSupplier;
if (healthCheckerFactory == null) {
healthCheckerSupplier = null;
} else {
@@ -154,14 +155,14 @@
private final LoadBalancerObserver<ResolvedAddress> loadBalancerObserver;
private final int linearSearchSpace;
@Nullable
- private final Supplier<HealthChecker> healthCheckerFactory;
+ private final Supplier<HealthChecker<ResolvedAddress>> healthCheckerFactory;
@Nullable
private final HealthCheckConfig healthCheckConfig;
DefaultLoadBalancerFactory(final String id, final LoadBalancingPolicy loadBalancingPolicy,
- final int linearSearchSpace, final HealthCheckConfig healthCheckConfig,
+ final int linearSearchSpace, final HealthCheckConfig healthCheckConfig,
final LoadBalancerObserver<ResolvedAddress> loadBalancerObserver,
- final Supplier<HealthChecker> healthCheckerFactory) {
+ @Nullable final Supplier<HealthChecker<ResolvedAddress>> healthCheckerFactory) {
this.id = requireNonNull(id, "id");
this.loadBalancingPolicy = requireNonNull(loadBalancingPolicy, "loadBalancingPolicy");
this.loadBalancerObserver = requireNonNull(loadBalancerObserver, "loadBalancerObserver");
Index: servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthIndicator.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthIndicator.java b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthIndicator.java
--- a/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthIndicator.java (revision 34db3ac6fde1fd333efc146b37b70506642e51f0)
+++ b/servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/HealthIndicator.java (date 1705107858564)
@@ -1,5 +1,5 @@
/*
- * Copyright © 2023 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2024 Apple Inc. and the ServiceTalk project authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
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:
We want to make health checking more module in
DefaultLoadBalancer.
Modifications:
Result:
A modular way to support health checking.