Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

Add route for retrying failed messages before sending to DLQ #32

Merged
merged 6 commits into from
Apr 4, 2016

Conversation

alechenninger
Copy link
Contributor

As part of auditing how inbound messages will work, I am reworking the intended path for retries a bit.

Originally, I had (naively) hoped we will be able to generalize these enough such that failure scenarios (specifically around locking) would be predictable, but seeing our business logic this is not the case (surprise). So, instead of making a Message base which did some error handling, I opted to make routes which deal with messages more flexible WRT error handling.

Immediately you may notice I am not using Camel's error handling directly. I wanted to, but I could not make it fit naturally. The main reason is that Camel's error handling redelivery redelivers the exchange as it existed before the processor that failed, at the processor that failed. Because we are processing messages in batch in one exchange, this would cause already succeeded messages to be reprocessed. This is not fatal, but it is easy enough to prevent this case that I think it is worth trying. Secondarily, Camel's error handling works expecting exchanges which throw exceptions, and in our routes we deal with failure in batches, and retain more context about the failures, so we use collections of failure messages which can be routed instead of throwing exceptions (this already existed prior to this PR). Camel's error handling fits more naturally when an exchange represents one individual message, not a batch. When using a route with a loop() processor instead I found much less friction.

I also got rid of the RecoverableException idea, which is no longer necessary, and would've been a surprising contract to follow given the variety of errors that can happen when processing inbound messages. I think ultimately error handling behavior will be more effective and predictable letting message processing throw whatever exception and retrying always. If we wanted, we could easily customize which kinds of exceptions cause retries, but I think for now we can put this off for later.

Many exceptions (if not most) are recoverable, and it is burdensome to
impose the contract of wrapping exceptions in RecoverableException when
"necessary" (when would it be necessary?).
private final Duration processTimeout;
private final String deadLetterUri;

private static final String NEXT_RETRY_ATTEMPT_NUMBER_PROPERTY = "eventHandlerFailedMessageNextRetryAttemptNumber";
Copy link
Member

Choose a reason for hiding this comment

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

How about just "current attempt number"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (went with "nextAttemptNumber" since current may be a little misleading)

@kahowell
Copy link
Member

kahowell commented Apr 4, 2016

Overall, LGTM, I'd like to explore at some point in the future organizing the retry logic for readability-sake, but as stated in a comment, we can handle as a backlog item. Merge when ready.

@alechenninger
Copy link
Contributor Author

organizing the retry logic for readability-sake

Would you mind opening an issue labeled under refactor to elaborate a little bit? Is there anything small we can do now?

@kahowell
Copy link
Member

kahowell commented Apr 4, 2016

Is there anything small we can do now?

Can't think of anything off the top of my head.

@alechenninger
Copy link
Contributor Author

Thank you!

@alechenninger alechenninger merged commit 8f6ba89 into master Apr 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants