gnodet commented on PR #29564: URL: https://github.com/apache/camel/pull/29564#issuecomment-6820609342
Thanks @davsclaus, addressed in 3f2056b8cf63: **1. CAMEL-26663 regression test** — the adapted test `checkOptimisticRemoveKeepsTheGroupWhenTheTransactionFails` was added in the previous commit (c6380db). The `RollbackAfterRemoveRepository` subclass rolls the transaction back after the conditional `tCache.remove()` succeeds — simulating a commit failure — and asserts that the entry is restored to the operational map (no group lost, no recovery entry written). `checkOptimisticRemoveOfAStaleExchangeFailsWhenTheCleanupFails` was removed as the cleanup path it covered no longer exists in the transaction model. **2. JIRA reference** — the code comment in `remove()` and the test Javadoc both cited CAMEL-26395 (the Redis issue) instead of CAMEL-26663 (the Hazelcast fix). Fixed in 3f2056b8. The PR description was also updated. **3. `remove()` Javadoc** — updated in c6380db to describe the unified transactional approach for both optimistic and non-optimistic paths. **4. ONE_PHASE vs TWO_PHASE** — good question. The pessimistic path already uses ONE_PHASE and so does this PR for consistency. With ONE_PHASE, a member crash during the commit phase can leave the two maps partially applied (the entry removed from `cache` but not yet in `persistedCache`, or vice versa), which could mean a loss in that rare window. TWO_PHASE would eliminate that via a prepare phase: in Hazelcast, `TWO_PHASE` stores the prepared transaction log in a backup member, so if the coordinator crashes after `prepare` but before `commit`, another member can recover and complete the commit. For a loss-averse `useRecovery=true` path that is a meaningful improvement. Happy to switch to TWO_PHASE here and in the pessimistic path if you think the extra overhead is worth it. As you note, the rollback on crash happens automatically due to the timeout (default 2 min), so for practical purposes both modes are safe — the difference is the width of the window between prepare and co mmit. **5. Optional concurrent-add test** — skipping for now; the core correctness properties are covered by the existing tests. Happy to add it if you think it adds meaningful coverage. _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]
