ramu11 commented on PR #28730: URL: https://github.com/apache/camel/pull/28730#issuecomment-6599766884
> Re-review of `2ce2479dd3..a7de0ba5b2`. Thanks @ramu11, the three points from my last review are addressed: > > * A non-Kafka wireTap or seda target no longer marks the shared metadata, and `filteredRecordWithNonKafkaWireTapStillCommits` pins it. > * An `onException(...).handled(true)` Kafka DLQ works and has an IT. > * The startup check follows sub-routes, rejects recipientList, routingSlip and dynamicRouter, and treats kamelet and unresolved targets as Kafka. > > Both nits are fixed, and the cache concern from gnodet-bot is handled: route events clear the cache, and `rechecksNonKafkaHandoffWhenRouteIsAddedAtRuntime` covers it. CI is green. > > 1. [Important] Removing the `isDeadLetterChannel()` filter also puts doCatch bodies, and `onException(...).continued(true)` blocks, in the failure-handler window, because `CatchProcessor` fires the same FailureHandling/Handled events (`CatchProcessor.java:205/210`). This changes two things that worked on `2ce2479dd3`: > > * Once the source-output transaction is open, a send inside doCatch to the same producer is rejected by `validateExactlyOnceDestination` (`KafkaProducer.java:740-748`). `isActiveExactlyOnceSendRetry` does not help, because CatchProcessor sets the failure endpoint. The record then replays forever. Before, the send joined the source transaction. The new doCatch IT only uses a separate producer, so it does not catch this. > * A transaction first opened inside doCatch is flagged `deadLetterTransaction` (`KafkaProducer.java:609`). If a later step fails and a non-Kafka DLC handles it, `KafkaTransactionSynchronization.java:155` commits the partial output instead of taking the offset-only path. > > doCatch is local recovery and the route continues afterwards, so I think its sends belong in the source transaction. One way to restore that: skip the non-DLC events while `ExchangePropertyKey.EXCEPTION_HANDLED` is set, since only CatchProcessor sets it, for the catch body. Could you add an IT that sends from doCatch to the source producer? For `continued(true)` blocks, please either handle them the same way or document that they must not use the source producer. > 2. [Suggestion] Negative reachability results are never cached (`KafkaRecordStreamingProcessor.java:383-401`). So every send from an exactly-once exchange to a non-Kafka seda target walks all routes and the route model, on the notifier hot path and again in `hasAsynchronousHandoff`. Route events now clear the cache (`:266`), so caching `false` is safe as well, and the "never cache false" comment at `:396` can go. > 3. [Nit] `kafka-component.adoc:644-648`: the hand-off paragraph should say which targets count as "can reach Kafka": unresolved targets, `kamelet:` targets, and targets that use recipientList, routingSlip or toD. A filtered record sent there still replays. The wireTap paragraph already says this. > > _Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying._ Thanks for the review. Addressed all three points, including the `doCatch` and `continued(true)` transaction handling, negative reachability caching with route-event invalidation, and documentation updates. Added regression coverage. Verified with `mvn -pl components/camel-kafka -am verify` — BUILD SUCCESS. Pushed in `2bdcd1176976`. -- 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]
