ramu11 commented on PR #28730: URL: https://github.com/apache/camel/pull/28730#issuecomment-6540718434
> Thanks @ramu11, this is a thorough round. I went through the delta against my review and @Croway's: > > * `exactlyOnce=true` now needs an explicit `transactionalId`. > * Batching, `seekTo`, `ResumeStrategy` and `consumersCount > 1` are rejected at startup. > * Filtered records commit their offsets from the consumer. > * The metadata lives in an exchange property, and a claim binds it to the source exchange. > * Exactly-once synchronizations no longer allow handover, and the slot is released with the id captured at start. > * A rollback-only or aborted transaction now replays the record. > * The producer-side unit tests and the regression IT cover the scenarios. > > All of my earlier points and Croway's #1-#8 look addressed in the code. > > One remaining way to lose a record: > > * **The send-failure marker survives a redelivery.** `KafkaExactlyOnceMetadata.recordSendFailure` keeps the first failure in `CamelKafkaExactlyOnceSendFailure`, and nothing ever removes it. Suppose attempt 1 fails inside `startKafkaTransaction` (for example `initTransactions()` times out while the producer is recreated, KafkaProducer.java:627) and the error handler redelivers. Attempt 2 claims, sends and succeeds. In `KafkaTransactionSynchronization.onDone` (lines 89-97) the exchange has no exception but still has the marker, so `commitOffsetsAfterHandledSendFailure` aborts the good output and commits the source offset: the record is lost. Could you clear the marker when a new synchronization is registered (a failure before that point does not belong to the transaction), or better, keep the send-failure flag on the `KafkaTransactionSynchronization` instance instead of the exchange? A unit test with a failed first start and a successful redelivery would cover it. > > Smaller: > > * `recordSendFailure` also runs for producers that are not exactly-once (KafkaProducer.java:431 and :522, KafkaProducerCallBack.java:89), so every failed Kafka send now leaves a `Throwable` exchange property. Please only record it when the exchange carries the exactly-once metadata. > * Doc: a Kafka destination that is not exactly-once is rejected at runtime, not at startup, so the record replays (and a DLC runs again) until the configuration is fixed. Worth saying. > * The batch-path exactly-once code (KafkaRecordBatchingProcessor.java:271-273, `KafkaExactlyOnceMetadata.fromExchanges`) is unreachable now that batching is rejected. It can go. > * The doc lists the rejected options twice (kafka-component.adoc around lines 598 and 601). > > _Claude Code on behalf of Claus Ibsen. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying._ Thanks for the review. I've addressed the comments in commit `6d46b1d5b2fb`. - **Stale failure marker:** Clear the previous send-failure marker when a new exactly-once synchronization is successfully registered. Extended the producer test to cover a failed first transaction start followed by a successful retry. - **Non-EOS producer paths:** `recordSendFailure` now records failures only for exchanges carrying exactly-once metadata. Added a unit test for a non-exactly-once exchange. - **Runtime destination validation:** Updated the component documentation to clarify that a destination without exactly-once support is rejected when the exchange is processed, not at route startup, and explain the replay/error-handler implications. - **Batching path:** Removed the unreachable batch exactly-once metadata handling and the now-unused `fromExchanges()` method, since batch processing is rejected for this mode. - **Documentation cleanup:** Removed the duplicate rejected-options explanation from both documentation copies. - Also removed the redundant `exchange.setException(null)` call and its corresponding test assertion. The changes are pushed to the PR branch. Thanks! -- 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]
