Closed alechenninger closed 8 years ago
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.
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?
Is there anything small we can do now?
Can't think of anything off the top of my head.
Thank you!
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.