allthingssecurity commented on PR #29584: URL: https://github.com/apache/camel/pull/29584#issuecomment-6794256004
Thanks for the review. I answer each point below. Points 3 and 4 are in 306ae6f0bdcd. 1. camel-kafka exactlyOnce: confirmed from the code. A route called from the `doTry` redelivers a failed send. `KafkaProducer` rejects the retry in the open transaction by setting `rollbackOnly` plus `CamelKafkaExactlyOnceRetryRollback`, and `CatchProcessor` sets `rollbackOnly` again after the block. `KafkaTransactionSynchronization` skips the rollback of a rejected retry only when the exchange is failure handled, so with this PR the transaction is aborted and the record is replayed. A send that keeps failing is replayed every time. The in-route `doTry` never rejects a retry and still takes the offsets-only path. A camel-kafka JIRA linked from CAMEL-26447 is drafted and will be filed shortly. The fix it suggests: treat a rejected retry as handled when the exchange has no exception left, so either an error handler or a `doCatch` handled it. The test would be a variant of `KafkaTransactionSynchronizationTest.handledInTransactionRedeliveryRejectionCommitsOffsetsAfterAbortingOutput` w ith `isFailureHandled()` false. I kept it out of this PR, as it belongs in camel-kafka with its own test. It should go in before 4.24.0. 2. Redelivery counter: a JIRA is drafted, linked from CAMEL-26447, and will be filed shortly. A quick test confirmed the example. The called route redelivered 2 times, the calling route has `maximumRedeliveries(3)`, and a later step got 1 redelivery instead of the 3 it gets with the in-route `doTry`. The description now says the JIRA is drafted. I'll put the key there once it is filed. 3. I removed the snapshot and the restore of the "handled by the error handler" mark. Both marks are cleared when the `doCatch` takes over, and neither is set again, so the exchange never carries `errorHandlerHandled=true` without `failureHandled`. A failure that the block handles itself is still marked by its own error handler. New test `doCatchLeavesNoHandledMarkOfTheCaughtFailure`: the `doTry` fails with both marks set, and after the `doCatch` neither is set. It fails on the first commit, where the true mark was set again. One case does reach that branch, from reading the code: an aggregation strategy (such as the `shareUnitOfWork` one) that combines a part whose failure was handled with a part that failed, inside the `doTry`. There the restored mark stopped the route after the `doTry`. Now the route goes on after the `doCatch`, as for any caught failure. The description says so. 4. Yes, the Idempotent Consumer advice is enough. The guide no longer mentions `setFailureHandled`. It now says that an exception thrown from the `doCatch` block removes the message id (with `removeOnFailure`, the default), so the message is processed again when it comes again. `doCatchCanMarkTheExchangeAsFailureHandled` is replaced by `rethrowInTheDoCatchRemovesTheIdempotentId`, which tests that advice in both variants. Tests: - `DoCatchFailureHandledMarkTest`: 300/300 over 20 runs in one JVM. - `DoCatchCalledRouteAfterUnhandledCalleeTest`: 460/460 over 20 runs. - camel-core suite: 9164 run, 1 error. The error is `FileConsumerAsyncCommitRaceTest`, which has no doTry; its JUnit extension context failed to close, and it passed 10/10 when run again. - camel-management doTry, redelivery and failures-handled tests: 21 run, 0 failures. The description is updated. _Claude Code on behalf of allthingssecurity_ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
