Do not retry some exceptions #231
Conversation
Pull Request Test Coverage Report for Build 2777Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 2794Details
💛 - Coveralls |
| boolean doNoRetry = Objects.nonNull(exceptions) && | ||
| !exceptions.isEmpty() && | ||
| Objects.nonNull(error) && | ||
| exceptions.contains(error.getClass()); |
There was a problem hiding this comment.
Are you sure about this? I think error.getClass() will always be a MessagingException - its cause will be the real exception (e.g. DoNotRetry2Exception)
There was a problem hiding this comment.
However, fixing this then causes the test to BootDoNotRetryTest to fail.
There was a problem hiding this comment.
ok, it's a bit more complicated:
If an exception is thrown in the handler, it will be wrapped by a MessagingException, and we have to check if error.getCause().getClass() is in the set of exceptions.
If an exception is thrown in middleware, then it will not (necessarily) be a MessagingException.
I could live with throwing (or re-throwing) only in the middleware, but I'm not sure if this is a good expectation.
There was a problem hiding this comment.
I see, you're right. In such cases we can check two things if it contains the actual class or it contains the cause. That should work, let me know if you think otherwise
| class BootDoNotRetryTest extends RetryTests { | ||
|
|
||
| @Test | ||
| void taskIsNotRetried() throws TimedOutException { |
There was a problem hiding this comment.
I'm not sure this test is correct:
Even though the job throws a DoNotRetryException exception, I can see that the job is being executed several (I think 4) times.
There was a problem hiding this comment.
let me investigate this, not sure how did it pass. It should have failed
| @@ -270,18 +274,19 @@ private int getMaxRetryCount(RqueueMessage rqueueMessage, QueueDetail queueDetai | |||
| : rqueueMessage.getRetryCount(); | |||
| } | |||
|
|
|||
| private void handleFailure(JobImpl job, int failureCount) { | |||
| private void handleFailure(JobImpl job, int failureCount, Throwable throwable) { | |||
There was a problem hiding this comment.
don't we need to check for getDoNotRetry() classes here too?
There was a problem hiding this comment.
We don't need here as this is just to get the stop flag for backoff method
There was a problem hiding this comment.
yes we need that here too, my bad.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: