Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add fallback strategy to CircuitBreakerClient #4818

Merged
merged 8 commits into from Jun 14, 2023
Merged

Add fallback strategy to CircuitBreakerClient #4818

merged 8 commits into from Jun 14, 2023

Conversation

my4-dev
Copy link
Contributor

@my4-dev my4-dev commented Apr 10, 2023 •
edited

Motivation:

Described in #4800

Modifications:

  • Add recover method to CircuitBreakerClientBuilder in order to set a fallback logic via a builder method
  • Add a new API to CircuitBreakerClientHandler to check which exception is a circuit breaker exception

Result:

ikhoon
ikhoon reviewed May 11, 2023
View reviewed changes
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClient.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClient.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClientBuilder.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClientHandler.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClientBuilder.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClientBuilder.java Outdated Show resolved Hide resolved
@my4-dev
Copy link
Contributor Author

my4-dev commented May 12, 2023

@ikhoon : Thank you for your advice! I've added the recover method to CircuitBreakerRpcClientBuilder and applied your advice.

ikhoon
ikhoon reviewed May 16, 2023
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly, looks good.

core/src/main/java/com/linecorp/armeria/client/circuitbreaker/AbstractCircuitBreakerClient.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/AbstractCircuitBreakerClient.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/AbstractCircuitBreakerClient.java Outdated Show resolved Hide resolved
...ain/java/com/linecorp/armeria/client/circuitbreaker/AbstractCircuitBreakerClientBuilder.java Show resolved Hide resolved
...ain/java/com/linecorp/armeria/client/circuitbreaker/AbstractCircuitBreakerClientBuilder.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClient.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClient.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerRpcClient.java Outdated Show resolved Hide resolved
...src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerRpcClientBuilder.java Outdated Show resolved Hide resolved
@ikhoon ikhoon added this to the 1.24.0 milestone May 16, 2023
@my4-dev
Copy link
Contributor Author

my4-dev commented May 17, 2023

@ikhoon : Thanks, I've applied all of your advice.

ikhoon
ikhoon approved these changes May 24, 2023
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @my4-dev! Left minor comments about annotations.

...ain/java/com/linecorp/armeria/client/circuitbreaker/AbstractCircuitBreakerClientBuilder.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClientBuilder.java Outdated Show resolved Hide resolved
...src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerRpcClientBuilder.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/AbstractCircuitBreakerClient.java Outdated Show resolved Hide resolved
@my4-dev
Copy link
Contributor Author

my4-dev commented May 24, 2023

Thanks for reviewing. I fixed it!

trustin
trustin approved these changes May 30, 2023
View reviewed changes
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍👍

jrhee17
jrhee17 approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 Thanks @my4-dev 🙇 👍 🚀

minwoox
minwoox reviewed Jun 2, 2023
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!
Left some nits. 😄

...ain/java/com/linecorp/armeria/client/circuitbreaker/AbstractCircuitBreakerClientBuilder.java Outdated
this.fallback = fallback;
return this;
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this).omitNullValues()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We also need to add fallback to this method.
Or we can just remove toString() method of builders, which I prefer. 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toString() method is removed.

core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClient.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerClientBuilder.java Outdated Show resolved Hide resolved
...src/main/java/com/linecorp/armeria/client/circuitbreaker/CircuitBreakerRpcClientBuilder.java Outdated Show resolved Hide resolved
minwoox
minwoox approved these changes Jun 13, 2023
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, @my4-dev! 👍 🙇

@minwoox minwoox merged commit 660b2c3 into line:main Jun 14, 2023
13 checks passed
@minwoox
Copy link
Member

minwoox commented Jun 14, 2023

Thanks, again @my4-dev!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikhoon ikhoon ikhoon approved these changes

@trustin trustin trustin approved these changes

@jrhee17 jrhee17 jrhee17 approved these changes

@minwoox minwoox minwoox approved these changes

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.

Add fallback strategy to CircuitBreakerClient
5 participants
@my4-dev @minwoox @trustin @ikhoon @jrhee17