ramu11 commented on PR #28730: URL: https://github.com/apache/camel/pull/28730#issuecomment-6542357486
> **TL;DR:** Thanks @ramu11, every point from the last round is in, but one way to lose a record is left: a redelivery inside the already-open transaction (`KafkaProducer.java:689` + `KafkaTransactionSynchronization.java:93`). 2 minor points. > > All of these look good: > > * The marker is cleared when a new synchronization registers. > * `recordSendFailure` now only records for exchanges with exactly-once metadata. > * The runtime-rejection doc is in. > * The batch dead code is removed. > * The doc duplicate is removed. > * The `setException(null)` nit is fixed. > > **Remaining record loss: redelivery in the same transaction.** Clearing the marker at `KafkaProducer.java:642` covers a failure before the synchronization registers. Here is the case it misses: > > 1. Attempt 1 begins the transaction and registers the synchronization, then the send fails and the marker is set. > 2. The error handler redelivers. `uow.isTransactedBy(transactionId)` is true, so we take the "Using existing kafka transaction" branch, and the marker stays. > 3. Attempt 2 succeeds. > 4. `onDone` sees no exception plus the marker. It aborts the good output and commits only the source offset, so the record is lost. > > A retry in the same transaction can succeed: in kafka-clients 4.3.1 `KafkaProducer.doSend`, only an `ApiException` moves the transaction into the error state. A `SerializationException` (for example a schema-registry hiccup in the serializer) or a Camel-side `createRecord` failure leaves it usable. > > Could we, in the existing-transaction branch, when the marker is present, `abortTransaction()` + `beginTransaction()` and clear the marker? That would also make redelivery work after an `ApiException`. Alternatively we could reject redelivery inside an exactly-once transaction. A unit test with a failed send, a successful redelivery (`isTransactedBy=true`) and then `onDone` expecting `commitTransaction` (no abort) would cover it. > > Smaller: > > * Doc (adoc ~612-614): with `.to(eosSink).to(nonEosKafka)` and a DLC that handles the error, the marker set by the second producer sends `onDone` down the skip path. The record is skipped rather than replayed. Worth stating, or replaying in that case. > * The new retry test only verifies `removeProperty` on a mock. Asserting the commit-vs-skip outcome through the synchronization would make it stronger. > > _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 follow-up review. Addressed the remaining points in commit `f9d92812c818`. - **Same-UoW redelivery:** Rejects retries after a send failure within an active transaction and marks the exchange rollback-only. This aborts the entire transaction and allows source-record replay in a fresh transaction. - **Non-EOS destination:** Clarified the handled-failure and source-offset skip behavior in both documentation copies. - **Regression coverage:** Added a test verifying retry rejection, transaction abort, and no source-offset commit. **Validation passed:** Kafka producer tests and all reported Maven builds completed successfully. Changes are committed and pushed to the PR branch. Thanks for the review! -- 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]
