-
Notifications
You must be signed in to change notification settings - Fork 38.4k
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.
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
Introduce minimal retry functionality as a core framework feature #34716
base: main
Are you sure you want to change the base?
Conversation
This commit introduces a minimal core retry feature. It is inspired by Spring Retry, but redesigned and trimmed to the bare minimum to cover most cases.
92f257e
to
baf3703
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fmbenhassine,
First and foremost, thanks for putting this PR together! 👍
I took a quick look, added a few comments, and asked a few questions (as food for thought).
And we will also discuss the proposal within the team.
Cheers,
Sam
spring-core/src/main/java/org/springframework/core/retry/RetryOperations.java
Outdated
Show resolved
Hide resolved
spring-core/src/main/java/org/springframework/core/retry/RetryPolicy.java
Outdated
Show resolved
Hide resolved
* Return the maximum number of retry attempts. | ||
* @return the maximum number of retry attempts | ||
*/ | ||
int getMaxAttempts(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the description of this PR, you state the following.
It is also focused only on stateless retries for now, but can be extended with other retry operations as well.
For future extension, do you envision extending the contract of RetryPolicy
?
If so, how?
If not, how do you envision extending the feature set "with other retry operations"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you envision a user possibly being able to implement a retry policy conditionally based on the actual exception thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't exclude extending the contract of RetryPolicy
with anything that we think essential for this first iteration. The list of exceptions to retry/exclude could be part of the retry policy contract.
By "can be extended with other retry operations" I was referring to stateful retries. Stateful retries require some sort of context (similar to the one currently in Spring Retry). But instead of overwhelming the RetryOperations
interface with both stateless and stateful operations, I envision stateful retries to be defined in a specialization of RetryOperations
(something like StatefulRetryOperations
with additional methods dealing with a context). I am trying to follow the ISP here, as most users won't probably need stateful retires, and therefore should not be forced to implement those methods. Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a change set that enriches the retry policy with a method that allows user to provide a predicate on the exception and specify the retry condition. I believe a predicate is more flexible than two lists of exceptions to include/exclude as it is currently in Spring Retry.
spring-core/src/main/java/org/springframework/core/retry/RetryOperations.java
Outdated
Show resolved
Hide resolved
spring-core/src/main/java/org/springframework/core/retry/RetryTemplate.java
Show resolved
Hide resolved
spring-core/src/main/java/org/springframework/core/retry/RetryTemplate.java
Outdated
Show resolved
Hide resolved
- Fix Javadoc indentation - Fix typo
Thank you for the review, @sbrannen ! I added a couple of inline comments and I will update the PR accordingly. |
- Add specific retry exception - Add RetryCallback interface with a logical name for diagnostics purpose - Improve RetryPolicy with ability to specify which exceptions to retry
I updated the PR with the following changes:
I find this new version to be more coherent and consistent with other templates in terms of structure and API. Looking forward to the team's thoughts on this! |
This commit introduces a minimal core retry feature in Spring Framework. It is inspired by Spring Retry, but redesigned and trimmed to the bare minimum to cover most cases.
The main entry point is the
RetryTemplate
class, which is designed in a similar fashion to other templates provided by Spring. The package is minimal on purpose to keep its usage as simple as possible (see examples inRetryTemplateTests
). It is also focused only on stateless retries for now, but can be extended with other retry operations as well.