Add resilience4j integration for circuitbreaker #4139

jrhee17 · 2022-03-14T05:34:43Z

Motivation:

Resilience4j provides utility patterns that are useful especially for microservices.
This PR attempts to integrate Resilience4j's circuit breaker utility into Armeria.

Modifications:

jdk 17 is required for resilience4j2.
- :examples, :resilience4j modules are now built to target >= jre 17
- tests for the above modules are not run for builds < jre 17.
- shadow-gradle-plugin has been bumped to 7.1.2, and the module name has been updated to gradle.plugin.com.github.johnrengelman:shadow.
  - (Breaking change) users of gradle-scripts now must define a dependency of gradle.plugin.com.github.johnrengelman:shadow instead of com.github.jengelman.gradle.plugins:shadow to shade dependencies.
Added a missing factory method CircuitBreakerRpcClient#newDecorator(CircuitBreakerClientHandler<RpcRequest> handler, CircuitBreakerRuleWithContent<RpcResponse> ruleWithContent) so that resilience4j2 module can support rpc decorators.
A new resilience4j2 module has been introduced.
- Resilience4jCircuitBreakerMappingBuilder which is analogous to CircuitBreakerMappingBuilder has been introduced.
- Resilience4jCircuitBreakerMapping which is analogous to CircuitBreakerMapping has been introduced.
- Resilience4JCircuitBreakerClientHandler and Resilience4JCircuitBreakerCallback have been introduced to use resilience4j's CircuitBreaker in conjunction with CircuitBreakerClient.
- Resilience4jCircuitBreakerFactory has been introduced for users who may want to customize how CircuitBreaker is generated based on host, path, and method.
  - Resilience4jCircuitBreakerUtils is added to hide the default implementation of Resilience4jCircuitBreakerFactory
- FailedCircuitBreakerDecisionException has been added to use as the default exception when a Throwable is not available for CircuitBreaker#onError.
An :examples:resilience4j-spring module has been introduced
- This module is intended to be a playground for validating integrations with resilience4j-spring-boot2 and resilience4j-micrometer.
An it:resilience4j module has been added to verify rpc integration.

Result:

Closes Integration with 3rd party retry/circuit breaker libraries #1469

trustin · 2022-03-22T04:00:11Z

Looking forward to progress in this PR ❤️

heesuk-ahn · 2022-07-25T04:18:26Z

@jrhee17 hi, jrhee :)

I am interested in this feature. Do you have any updates by any chance?
Will it be released in version 1.19.0?

jrhee17 · 2022-07-25T14:28:19Z

@heesuk-ahn Hi! Sorry about the late reply. Let me increase priority in getting this feature merged 🙇

codecov · 2022-07-28T04:49:13Z

Codecov Report

Base: 74.08% // Head: 74.06% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (6ed4cab) compared to base (c34c6b5).
Patch coverage: 75.22% of modified lines in pull request are covered.

❗ Current head 6ed4cab differs from pull request most recent head eb982b3. Consider uploading reports for the commit eb982b3 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4139      +/-   ##
============================================
- Coverage     74.08%   74.06%   -0.03%     
- Complexity    18185    18241      +56     
============================================
  Files          1537     1549      +12     
  Lines         67469    67629     +160     
  Branches       8537     8545       +8     
============================================
+ Hits          49987    50089     +102     
- Misses        13419    13466      +47     
- Partials       4063     4074      +11

Impacted Files	Coverage Δ
.../client/circuitbreaker/CircuitBreakerListener.java	`25.00% <ø> (ø)`
...a/client/circuitbreaker/CircuitBreakerMapping.java	`100.00% <ø> (ø)`
...circuitbreaker/CircuitBreakerRpcClientBuilder.java	`0.00% <0.00%> (ø)`
.../circuitbreaker/DefaultCircuitBreakerCallback.java	`0.00% <0.00%> (ø)`
...ent/circuitbreaker/KeyedCircuitBreakerMapping.java	`69.23% <0.00%> (-17.44%)`	⬇️
...nt/circuitbreaker/CircuitBreakerClientBuilder.java	`72.22% <28.57%> (-27.78%)`	⬇️
...java/example/armeria/resilience4j/spring/Main.java	`33.33% <33.33%> (ø)`
...t/circuitbreaker/AbstractCircuitBreakerClient.java	`68.42% <50.00%> (-14.92%)`	⬇️
...cuitbreaker/Resilience4jCircuitBreakerMapping.java	`50.00% <50.00%> (ø)`
...reaker/KeyedResilience4jCircuitBreakerMapping.java	`69.56% <69.56%> (ø)`
... and 40 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

jrhee17 · 2022-08-30T14:14:50Z

Really sorry about the delay all 😅 I think this PR is finally in a reviewable state 🙏

jrhee17 · 2022-12-26T00:53:16Z

I believe this PR is (finally) ready for review. Please take a look at your convenience 🙏

ikhoon · 2022-12-26T09:33:32Z

build.gradle

-        if (task.path.startsWith(':examples:')) {
-            // Target Java 11 for examples.
-            options.release.set(11)
+        if (task.path.startsWith(':examples:') || task.path.contains(':resilience4j')) {


It is out of scope of this PR but we need a general solution to tell a module needs Java 17 to compile.
What do you think of introducing java17 flag for it? We may set both java and java17 or only java17 to Spring Boot 3 and Resilience4J projects.

Agreed! Let me work on this in a separate PR 😄

.../com/linecorp/armeria/resilience4j/circuitbreaker/FailedCircuitBreakerDecisionException.java

...java/com/linecorp/armeria/resilience4j/circuitbreaker/Resilience4jCircuitBreakerFactory.java

...n/java/com/linecorp/armeria/resilience4j/circuitbreaker/Resilience4jCircuitBreakerUtils.java

...corp/armeria/resilience4j/circuitbreaker/client/Resilience4JCircuitBreakerClientHandler.java

ikhoon · 2022-12-27T03:39:33Z

...corp/armeria/resilience4j/circuitbreaker/client/Resilience4JCircuitBreakerClientHandler.java

+            logger.warn("Failed to get a circuit breaker from mapping", t);
+            return null;
+        }
+        circuitBreaker.acquirePermission();


What do you think of catching CallNotPermittedException and wrapping it with FailFastException and UnprocessedRequestException?
FailFastException would be useful to set a retry rule and I found that there are some logics relying on FailFastException.
I guess we also need to wrap UnprocessedRequestException for

armeria/core/src/main/java/com/linecorp/armeria/client/circuitbreaker/DefaultCircuitBreakerClientHandler.java

Line 48 in e495f0d

throw new FailFastException(circuitBreaker);

I've wrapped with UnprocessedRequestException, but am unsure if we should generalize FailFastException to accept a resilience4j circuitbreaker..

I believe the general direction is to move away from custom circuitbreaker implementations to resilience4j, which will eventually make FailFastException obsolete

I think wrapping with UnprocessedRequestException is enough for the current logic/retry rules.

Let me know if you feel differently though 😄

Make sense. I found only one place where FailFastException is used internally that might be converted by FailFastException.

armeria/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/client/thrift/THttpClientDelegate.java

Lines 341 to 345 in fee87f8

if (cause instanceof TException ||

cause instanceof UnprocessedRequestException ||

cause instanceof FailFastException) {

return (Exception) cause;

}

By the way, there might be a certain reason that FailFastException isn't wrapped by UnprocessedRequestException. @minwoox Do you know the historical reason? Otherwise, is it a bug as I thought?

Had a chat internally and we decided to consider the change in Armeria 2.0.
Sorry for the incorrect direction. Could you revert the wrapping code? 🙇‍♂️
#4623

Reverted for DefaultCircuitBreakerClientHandler 🙇

ikhoon · 2022-12-27T04:06:05Z

...corp/armeria/resilience4j/circuitbreaker/client/Resilience4JCircuitBreakerClientHandler.java

+ *
+ * // for RpcRequest
+ * CircuitBreakerRuleWithContent rule = ...;
+ * CircuitBreakerClientHandler<RpcRequest> handler = Resilience4JCircuitBreakerClientHandler.of(


I'm not sure if the T type parameter for Request makes the API type-safe. It looks a bit verbose to use. ClientCircuitBreakerGenerator already takes Request as a parameter. How about removing the T type parameter from CircuitBreakerClientHandler and inline Request to the method argument?

CircuitBreakerCallback tryRequest(ClientRequestContext ctx, Request req);

I'm guessing if we do this we can't use the following pattern 😅 I've updated the javadocs though 🙏

final Function<? super HttpClient, CircuitBreakerClient> circuitBreakerDecorator = CircuitBreakerClient.newDecorator( // expects HttpRequest as the parameterized type, but is Request Resilience4JCircuitBreakerClientHandler.of(mapping), rule);

Couldn't we change CircuitBreakerClientHandler<HttpRequest> to CircuitBreakerClientHandler? It is still an unstable API.

armeria/core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClient.java

Line 116 in e495f0d

newDecorator(CircuitBreakerClientHandler<HttpRequest> handler,

I think you're right, looks much cleaner 👍 Good idea as always 🙇

ikhoon

Awesome work! 🚀🥳
Left a question on `UnprcoessedRequestException. All other things are minor.

...esilience4j-spring/src/test/java/example/armeria/resilience4j/spring/CircuitBreakerTest.java

...java/com/linecorp/armeria/resilience4j/circuitbreaker/Resilience4jCircuitBreakerFactory.java

settings.gradle

...corp/armeria/resilience4j/circuitbreaker/client/Resilience4JCircuitBreakerClientHandler.java

minwoox

Mostly looking good! 👍 👍 👍

core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClientHandler.java

.../com/linecorp/armeria/resilience4j/circuitbreaker/FailedCircuitBreakerDecisionException.java

...java/com/linecorp/armeria/resilience4j/circuitbreaker/Resilience4jCircuitBreakerFactory.java

...n/java/com/linecorp/armeria/resilience4j/circuitbreaker/Resilience4jCircuitBreakerUtils.java

.../linecorp/armeria/resilience4j/circuitbreaker/client/Resilience4JCircuitBreakerCallback.java

...corp/armeria/resilience4j/circuitbreaker/client/Resilience4JCircuitBreakerClientHandler.java

...m/linecorp/armeria/resilience4j/circuitbreaker/client/Resilience4jCircuitBreakerMapping.java

minwoox

Great work, @jrhee17! 👍 👍 👍

...java/com/linecorp/armeria/resilience4j/circuitbreaker/Resilience4jCircuitBreakerFactory.java

ikhoon · 2023-02-07T08:30:45Z

Still looks good. 👍

ikhoon · 2023-02-07T08:31:41Z

Thanks for your hard work. You finally made it! 🚀🥳

heesuk-ahn · 2023-02-13T05:28:44Z

wow ! 👀 thank you for your effort ! I will check this feature latest version :) 👍

The changes in line/armeria#4139 were not integrated back to upstream. I've verified that there aren't any more inconsistencies: ``` ~/Projects/gradle-scripts on main 1 % diff -r . ~/Projects/centraldogma/gradle/scripts Only in .: .git Only in /Users/jrhee17/Projects/centraldogma/gradle/scripts: .gitrepo Only in .: .gradle Only in .: .idea ```

trustin mentioned this pull request Mar 22, 2022

Integration with 3rd party retry/circuit breaker libraries #1469

Closed

jrhee17 added this to the 1.17.0 milestone Apr 14, 2022

jrhee17 modified the milestones: 1.17.0, 1.18.0 Jun 13, 2022

jrhee17 modified the milestones: 1.18.0, 1.19.0 Jul 22, 2022

jrhee17 force-pushed the feature/r4j-cb branch 2 times, most recently from 1a3cf23 to cc37a71 Compare July 27, 2022 15:13

jrhee17 force-pushed the feature/r4j-cb branch 4 times, most recently from 71b5bc5 to 3c067d9 Compare July 30, 2022 08:22

jrhee17 force-pushed the feature/r4j-cb branch 3 times, most recently from 1dbbc11 to 864330b Compare August 30, 2022 09:26

jrhee17 changed the title ~~[WIP] Add resilience4j integration for circuitbreaker~~ Add resilience4j integration for circuitbreaker Aug 30, 2022

jrhee17 force-pushed the feature/r4j-cb branch from 1aa99f7 to 29126d7 Compare August 30, 2022 13:22

jrhee17 marked this pull request as ready for review August 30, 2022 14:14

jrhee17 requested review from ikhoon, minwoox and trustin as code owners August 30, 2022 14:14

ikhoon modified the milestones: 1.19.0, 1.20.0 Aug 30, 2022

ikhoon added the new feature label Aug 31, 2022

jrhee17 mentioned this pull request Sep 14, 2022

Generalize CircuitBreakerClient to accept an arbitrary CircuitBreaker implementation #4426

Merged

jrhee17 added 4 commits December 25, 2022 22:13

minor fixes

c6df95f

use Resilience4JCircuitBreakerClientHandler for factory methods

072aedd

minor cleanups

d0aa793

minor cleanup

f0f66cf

ikhoon reviewed Dec 26, 2022

View reviewed changes

ikhoon reviewed Dec 27, 2022

View reviewed changes

jrhee17 added 4 commits December 27, 2022 15:44

address comments by @ikhoon

242a14e

handle test failure

1080fa6

update documentation

294c07b

remove parameterized type from handler

06f0d41

jrhee17 force-pushed the feature/r4j-cb branch from bf421a0 to 06f0d41 Compare December 27, 2022 13:30

jrhee17 added 2 commits January 3, 2023 18:14

Merge branch 'master' of github.com:line/armeria into feature/r4j-cb

2c38b2e

set java17 for example/it

faeb38f

ikhoon approved these changes Jan 9, 2023

View reviewed changes

jrhee17 added 3 commits January 12, 2023 22:36

address comments by @ikhoon

ca98541

default cb throws a failfastexception

a227623

fix failing test

2ff1064

jrhee17 force-pushed the feature/r4j-cb branch from 107446a to 2ff1064 Compare January 12, 2023 22:19

minwoox reviewed Feb 1, 2023

View reviewed changes

jrhee17 added 3 commits February 6, 2023 17:47

Merge branch 'master' of github.com:line/armeria into feature/r4j-cb

334a6b3

address comments by @minwoox

50db27a

add @UnstableApi for public apis

eb982b3

minwoox approved these changes Feb 6, 2023

View reviewed changes

...java/com/linecorp/armeria/resilience4j/circuitbreaker/Resilience4jCircuitBreakerFactory.java Show resolved Hide resolved

ikhoon merged commit ab7488b into line:master Feb 7, 2023

jrhee17 mentioned this pull request Apr 10, 2023

Use the updated shadow library line/gradle-scripts#147

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resilience4j integration for circuitbreaker #4139

Add resilience4j integration for circuitbreaker #4139

jrhee17 commented Mar 14, 2022 •

edited

Loading

trustin commented Mar 22, 2022

heesuk-ahn commented Jul 25, 2022

jrhee17 commented Jul 25, 2022

codecov bot commented Jul 28, 2022 •

edited

Loading

jrhee17 commented Aug 30, 2022

jrhee17 commented Dec 26, 2022

ikhoon Dec 26, 2022

jrhee17 Dec 27, 2022

ikhoon Dec 27, 2022

jrhee17 Dec 27, 2022

ikhoon Jan 9, 2023 •

edited

Loading

ikhoon Jan 11, 2023

jrhee17 Jan 12, 2023

ikhoon Dec 27, 2022

jrhee17 Dec 27, 2022

ikhoon Dec 27, 2022

jrhee17 Dec 27, 2022 •

edited

Loading

ikhoon left a comment

minwoox left a comment

minwoox left a comment

ikhoon commented Feb 7, 2023

ikhoon commented Feb 7, 2023

heesuk-ahn commented Feb 13, 2023

	if (cause instanceof TException \|\|
	cause instanceof UnprocessedRequestException \|\|
	cause instanceof FailFastException) {
	return (Exception) cause;
	}

Add resilience4j integration for circuitbreaker #4139

Add resilience4j integration for circuitbreaker #4139

Conversation

jrhee17 commented Mar 14, 2022 • edited Loading

trustin commented Mar 22, 2022

heesuk-ahn commented Jul 25, 2022

jrhee17 commented Jul 25, 2022

codecov bot commented Jul 28, 2022 • edited Loading

Codecov Report

jrhee17 commented Aug 30, 2022

jrhee17 commented Dec 26, 2022

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

ikhoon Jan 9, 2023 • edited Loading

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

jrhee17 Dec 27, 2022 • edited Loading

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon commented Feb 7, 2023

ikhoon commented Feb 7, 2023

heesuk-ahn commented Feb 13, 2023

jrhee17 commented Mar 14, 2022 •

edited

Loading

codecov bot commented Jul 28, 2022 •

edited

Loading

ikhoon Jan 9, 2023 •

edited

Loading

jrhee17 Dec 27, 2022 •

edited

Loading