Provide a way to detect unhealthy connections #5763

ikhoon · 2024-06-14T11:14:00Z

Motivation:

Currently, there is no extension point to detect errors for specific connections and terminate connections that are unhealthy.

Related: #5717 #5751

API design:

OutlierDetector is similar to CircuitBreaker, but the state is simpler, only one direction, and optimized for ephemeral resources such as connections and streams.

OutlierDetector determines whether the target is an outlier based on onSuccess() and onFailure() events.
OutlierDetectingRule is used to decide whether a request fails.
OutlierDetectionDecision is the result of OutlierDetectingRule.
- OutlierDetectionDecision.FATAL is a special result that can immediately mark the target as an outlier.

Example:

OutlierDetectingRule rule =
  OutlierDetectingRule
    .builder()
    .onServerError()
    .onException(IOException.class)
    .onException(WriteTimeoutException, OutlierDetectionDecision.FATAL)
    .build();

OutlierDetection outlierDetection = 
  OutlierDetection
    .builder(rule)
    .counterSlidingWindow(10_seconds)
    .counterUpdateInterval(1_seconds)
    .failureRateThreshold(0.5)
    .build();

ClientFactory
  .builder()
  // Apply the OutlierDetection to detect and close unhealthy connections
  .connectionOutlierDetection(outlierDetection)

Modifications:

Add OutlierDetector, OutlierDetectingRule and OutlierDetectionDecision and their implementations to common.outlier package.
Move EventCounter, EventCount and SlidingWindowCounter to common.util package and expose EventCounter and EventCount as public APIs to minimize duplication.
- SlidingWindowCounter can be created with EventCounter.ofSlidingWindow(...).
Unlike CircuitBreakerRule, OutlierDetectingRule returns a decision synchronously and is simplified to look at headers and causes. Because:
- I didn't see content is necessary to detect an outlier.
- The response status and exception type are sufficient information to determine the result.
Create an OutlierDetector in HttpSessionHandler
- A hook applying OutlierDetector is added right after HttpSessionHandler.invoke() is called.
Deprecating) CircuitBreakerListener.onEventCountUpdated(String,.circuitbreaker.EventCount) has been deprecated in favor of CircuitBreakerListener.onEventCountUpdated(String,.util.EventCount).
Add ClientFactoryBuilder.connectionOutlierDetection(OutlierDetection) to detect unhealthy connection.
- This option is disabled by default.

Result:

You can now use OutlierDetection to detect unhealthy connections and close them gracefully.
Closes Outlier detection for connections #5751

Motivation: Currently, there is no extension point to detect errors for specific connections and terminate connections that are unhealthy. Related: line#5751 Modifications: - TBU Result:

github-actions · 2024-06-14T11:19:24Z

🔍 Build Scan® (commit: `76c38f1`)

Job name	Status	Build Scan®
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/a2l3qfp437e4g
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/rob6boeoowa5i
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/qb3yyvpwsxsgq
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/r6eldo3vnos2w
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/ds2dzip3zfckm
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/wpojdg464ddu2
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/fzw52exefj6xs

jrhee17

Looks good in terms of correctness. Left only nit comments 👍 👍 👍

core/src/main/java/com/linecorp/armeria/client/HttpSessionHandler.java

core/src/main/java/com/linecorp/armeria/common/outlier/OutlierDetectionBuilder.java

jrhee17 · 2024-06-18T06:17:51Z

core/src/main/java/com/linecorp/armeria/common/outlier/OutlierDetection.java

+ * {@link OutlierDetectingRule} and {@link OutlierDetector}.
+ */
+@UnstableApi
+public interface OutlierDetection {


Optional) would OutlierDetectorFactory be a clearer name?

I also thought about the name, OutlierConfig and OutlierContext.
OutlierDetectorFactory wasn't chosen since it had OutlierDetectingRule.

I'm not strong on the name. Let's listen to other folks' opinions and choose a better one.

OutlierDetection sounds OK to me, but OutlierDetectingRule could be renamed to OutlierRule?

OutlierDetectorFactory wasn't chosen since it had OutlierDetectingRule.

It seems like OutlierDetectingRule is used alongside OutlierDetector, and both are retrieved and created from this OutlierDetection. What do you think of adding OutlierDetectingRule to OutlierDetector instead?

outlierDetector.rule().decide(...)

core/src/main/java/com/linecorp/armeria/common/outlier/OutlierDetectingRuleBuilder.java

jrhee17 · 2024-06-18T06:25:59Z

core/src/main/java/com/linecorp/armeria/client/HttpSessionHandler.java

+                    outlierDetector.onSuccess();
+                    // The connection was marked as an outlier, but it's back to normal.
+                    if (outlierDetector.isOutlier()) {
+                        markUnacquirable();


Note: I would still recommend users (especially those using HTTP1) to use a RetryingClient as I think it's possible that requests that unluckily acquired this connection could be failed due to this call depending on the timing. Although I know the code path to markUnacquirable is already exposed to the user, I feel like the timing of this call is more unpredictable with this change.

e.g. If canSendRequest = false:

armeria/core/src/main/java/com/linecorp/armeria/client/AbstractHttpRequestHandler.java

Line 160 in 2219de1

id = session.incrementAndGetNumRequestsSent();

Good point. What do you think of explicitly executing this callback in the EventLoop to guarantee the execution order?

core/src/main/java/com/linecorp/armeria/common/outlier/OutlierDetectingRuleBuilder.java

core/src/main/java/com/linecorp/armeria/client/HttpSessionHandler.java

core/src/main/java/com/linecorp/armeria/common/outlier/OutlierDetectionBuilder.java

core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java

trustin · 2024-06-20T13:43:18Z

core/src/main/java/com/linecorp/armeria/common/outlier/OutlierDetectingRuleBuilder.java

+ * A builder for creating an {@link OutlierDetectingRule}.
+ */
+@UnstableApi
+public final class OutlierDetectingRuleBuilder {


Can we extend AbstractRuleBuilder?

They are similar but AbstractRuleBuilder is in the client package and ClientRequestContext is used while OutlierDetectingRuleBuilder uses RequestContext.

minwoox

Looks great! Left a few suggestions. 👍

core/src/main/java/com/linecorp/armeria/client/HttpSessionHandler.java

minwoox · 2024-07-02T05:29:22Z

core/src/main/java/com/linecorp/armeria/common/outlier/OutlierDetection.java

+ * {@link OutlierDetectingRule} and {@link OutlierDetector}.
+ */
+@UnstableApi
+public interface OutlierDetection {


OutlierDetectorFactory wasn't chosen since it had OutlierDetectingRule.

It seems like OutlierDetectingRule is used alongside OutlierDetector, and both are retrieved and created from this OutlierDetection. What do you think of adding OutlierDetectingRule to OutlierDetector instead?

outlierDetector.rule().decide(...)

ikhoon · 2024-07-04T02:58:19Z

It seems like OutlierDetectingRule is used alongside OutlierDetector, and both are retrieved and created from this OutlierDetection. What do you think of adding OutlierDetectingRule to OutlierDetector instead?

They are used together but the role of OutlierDetector and OutlierDetectingRule is different. OutlierDetector is designed as a utility. It may be used alone. So I didn't want to force users who only need OutlierDetector to implement OutlierDetectingRule.

minwoox · 2024-07-04T06:48:32Z

OutlierDetector is designed as a utility. It may be used alone.

Haven't thought about this case. Then, I'm fine with the current design. 👍

minwoox

I'm not sure that you want to use this for xDS but I found there's a mismatch with the Envoy implementation so I've left a question. PTAL. 😉

core/src/main/java/com/linecorp/armeria/client/HttpSessionHandler.java

minwoox · 2024-08-02T08:37:56Z

core/src/main/java/com/linecorp/armeria/common/outlier/DefaultOutlierDetector.java

+    private final EventCounter counter;
+    private final double failureRateThreshold;
+    private final long minimumRequestThreshold;
+    private volatile boolean isOutlier;


So if it once becomes an outlier, the client will never use it unlike Envoy bring it back after rejection time. Is that correct?
https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/outlier#ejection-algorithm

The outlier algorithm of Envoy is designed at the host (endpoint) level. Hosts do not tend to change for a long time. So, after ejection_time, sending requests again like a circuit breaker is necessary.

To provide the same functionality as Envoy, we may create an OutlierDetectingEndpointGroup using OutlierDetector.

If a host is determined as an outlier, the host is excluded from healthy endpoints for ejection_time.

After ejection_time, if the host is still in the endpoints, a new OutlierDetector is created for it.

ejection_time option will be provided by OutlierDetectingEndpointGroup

In fact, I implemented a similar EndpointGroup internally for the LINE push server. It would be better to modify the implementation and add it to Armeria.

Understood. 👍

minwoox

Thanks! 👍 👍 👍

Provide a way to detect unhealthy connections

569d0f5

Motivation: Currently, there is no extension point to detect errors for specific connections and terminate connections that are unhealthy. Related: line#5751 Modifications: - TBU Result:

ikhoon added the new feature label Jun 14, 2024

revert unintended changes

bb76df2

ikhoon mentioned this pull request Jun 17, 2024

[ISSUE-5715] Apply circuit breaker per connection #5764

Closed

ikhoon added 2 commits June 17, 2024 17:10

lint

770556f

fix the test failure

2ac5bb2

ikhoon added this to the 1.30.0 milestone Jun 17, 2024

ikhoon marked this pull request as ready for review June 17, 2024 10:17

ikhoon requested review from jrhee17, minwoox and trustin as code owners June 17, 2024 10:17

jrhee17 approved these changes Jun 18, 2024

View reviewed changes

ikhoon added 2 commits June 19, 2024 21:31

Address comments by @jrhee17

c599cbc

fix

3eef3b6

jrhee17 reviewed Jun 20, 2024

View reviewed changes

ikhoon added 2 commits June 20, 2024 10:31

early return and rnn

ae39d57

final

f58b49e

trustin reviewed Jun 20, 2024

View reviewed changes

minwoox reviewed Jul 2, 2024

View reviewed changes

ikhoon added 3 commits July 5, 2024 10:19

address comments

f505d2d

Merge branch 'main' into outlier-detection

4aa2059

checkstyle

229071f

minwoox reviewed Aug 2, 2024

View reviewed changes

ikhoon added 2 commits August 7, 2024 00:23

Add a comment

423e40c

change the comment location

af605f6

minwoox approved these changes Aug 7, 2024

View reviewed changes

Merge branch 'main' into outlier-detection

76c38f1

ikhoon merged commit dd14024 into line:main Aug 7, 2024
14 of 15 checks passed

ikhoon deleted the outlier-detection branch August 7, 2024 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to detect unhealthy connections #5763

Provide a way to detect unhealthy connections #5763

ikhoon commented Jun 14, 2024 •

edited

Loading

github-actions bot commented Jun 14, 2024 •

edited

Loading

jrhee17 left a comment

jrhee17 Jun 18, 2024

ikhoon Jun 19, 2024

trustin Jun 20, 2024

minwoox Jul 2, 2024

jrhee17 Jun 18, 2024

ikhoon Jun 19, 2024

trustin Jun 20, 2024

ikhoon Jul 4, 2024

minwoox left a comment

minwoox Jul 2, 2024

ikhoon commented Jul 4, 2024

minwoox commented Jul 4, 2024

minwoox left a comment

minwoox Aug 2, 2024

ikhoon Aug 6, 2024 •

edited

Loading

minwoox Aug 7, 2024

minwoox left a comment

Provide a way to detect unhealthy connections #5763

Provide a way to detect unhealthy connections #5763

Conversation

ikhoon commented Jun 14, 2024 • edited Loading

github-actions bot commented Jun 14, 2024 • edited Loading

🔍 Build Scan® (commit: 76c38f1)

jrhee17 left a comment

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

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

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon commented Jul 4, 2024

minwoox commented Jul 4, 2024

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon commented Jun 14, 2024 •

edited

Loading

github-actions bot commented Jun 14, 2024 •

edited

Loading

🔍 Build Scan® (commit: `76c38f1`)

ikhoon Aug 6, 2024 •

edited

Loading