gnodet commented on PR #29564: URL: https://github.com/apache/camel/pull/29564#issuecomment-6820833884
Thank you @davsclaus for the thorough review. Items 1–3 and 6 are now addressed across the last four commits: 1. **Put-fails test** — `checkOptimisticRemoveKeepsTheGroupWhenTheTransactionFails` was added in commit `c6380db` using a subclass that explicitly rolls back after the conditional remove, confirming the rollback restores the entry to the operational map (the CAMEL-26663 guarantee holds). The PR description has also been corrected. 2. **JIRA reference** — `CAMEL-26395` removed from the code comment and test Javadoc; only `CAMEL-26663` now remains where the Hazelcast loss window is referenced (commit `3f2056b`). 3. **`remove()` Javadoc** — updated in the same commit to reflect that both paths (optimistic and non-optimistic) now go through a Hazelcast transaction when `useRecovery` is true. 6. **Error message nit** — `IMap#remove` changed to `TransactionalMap#remove returned false (compare-and-delete found no matching entry)` in the transactional path, and `IMap#remove returned false (compare-and-delete found no matching entry)` in the non-transactional path (commit `6dc9b6c`). The test Javadoc "rolled back on restart" → "rolled back on the cluster side (owner failure or timeout)" was fixed in the same commit. **On ONE_PHASE vs TWO_PHASE (item 5):** You are correct that with `ONE_PHASE`, if the transaction owner member fails during the commit phase, the other participants cannot determine whether to commit or roll back — the cluster resolves this via timeout/owner-left detection, not on JVM restart. `TWO_PHASE` would be strictly safer here (the prepare phase locks the records cluster-wide, so a member failure during commit is recoverable via the prepared state), but it costs an extra network round-trip. The non-optimistic `remove()` already uses `ONE_PHASE`, and I am using the same type here for consistency. Given that the commit phase itself is very narrow and that `TWO_PHASE` adds measurable latency on the hot aggregation path, I'd prefer to keep `ONE_PHASE` for now and note it as a potential follow-up. That said, if you feel the no-loss guarantee warrants `TWO_PHASE` unconditionally, I can change it — it is a one-line switch. **Item 4 (optional concurrent-add test):** I'll leave this out for now to keep the scope focused on the race fix. It can be added as a follow-up if needed. _Hermes/forgebot on behalf of gnodet._ -- 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]
