Add rethrow flag #265
Add rethrow flag #265
Conversation
|
@ttulka Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@ttulka Thank you for signing the Contributor License Agreement! |
ttulka
changed the title
|
@garyrussell Any updates on this, please? |
|
Sorry; haven't had a chance to look at it yet; will try this week. |
| } | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * Delegates to an exception classifier. | ||
| * @param ex | ||
| * @return true if this exception or its ancestors have been registered as retryable. |
There was a problem hiding this comment.
Need @since 1.3.3 now that it is public.
| context.setAttribute(RetryContext.EXHAUSTED, true); | ||
| if (state != null && !context.hasAttribute(GLOBAL_STATE)) { | ||
| this.retryContextCache.remove(state.getKey()); | ||
| } | ||
| if (this.throwLastExceptionOnExhausted && retryPolicy instanceof SimpleRetryPolicy |
There was a problem hiding this comment.
This is a behavior change and could break existing projects that expect the recovery callback to be invoked for all exceptions. I think we need a new boolean noRecoveryForNotRetryable or similar.
There was a problem hiding this comment.
@garyrussell throwLastExceptionOnExhausted is true only if Retryable.rethrow is true, which is false per default. So, all the old code will have throwLastExceptionOnExhausted set to false and this new if branch will be skipped.
Could you please collaborate more if I miss something? Thanks!
There was a problem hiding this comment.
@ttulka Sorry for the delay. The problem is for users of the RetryTemplate (those not using @Retryable, which is very common).
If an existing application is using the template with throwLastExceptionOnExhausted then the recovery callback is still invoked for a not-retryable exception.
With your change, it will no longer be called; hence the new behavior needs to be controlled by a new property on the template rather than overloading the existing property to have 2 meanings.
There was a problem hiding this comment.
@garyrussell Thanks for clearing things up! I have committed the changes with the new flag.
this implements #264