gnodet-bot commented on code in PR #29564:
URL: https://github.com/apache/camel/pull/29564#discussion_r4549883300
##########
components/camel-hazelcast/src/main/java/org/apache/camel/processor/aggregate/hazelcast/HazelcastAggregationRepository.java:
##########
@@ -358,27 +362,65 @@ public void setHazelcastInstance(HazelcastInstance
hazelcastInstance) {
public void remove(CamelContext camelContext, String key, Exchange
exchange) {
DefaultExchangeHolder holder = DefaultExchangeHolder.marshal(exchange,
true, allowSerializedHeaders);
if (optimistic) {
- // put the exchange into the recoverable storage before removing
it from the operational storage: the two
- // maps cannot be updated atomically, and if the process stops in
between, the group is sent twice
- // instead of being lost
- if (useRecovery) {
- LOG.trace("Putting an exchange with ID {} for key {} into a
recoverable storage in an optimistic manner.",
- exchange.getExchangeId(), key);
- persistedCache.put(exchange.getExchangeId(), holder);
- LOG.trace("Put an exchange with ID {} for key {} into a
recoverable storage in an optimistic manner.",
- exchange.getExchangeId(), key);
- }
LOG.trace("Removing an exchange with ID {} for key {} in an
optimistic manner.", exchange.getExchangeId(), key);
- if (!cache.remove(key, holder)) {
- LOG.error(
- "Optimistic locking failed for exchange with key {}:
IMap#remove removed no Exchanges, while it's expected to remove one.",
- key);
- if (useRecovery) {
- removeFromRecoverableStorage(exchange.getExchangeId(),
key);
+ if (useRecovery) {
+ // Use a Hazelcast transaction to atomically apply the
conditional remove and the recovery write.
+ // A plain persistedCache.put(...) followed by
cache.remove(key, holder) (the pre-fix order) has two failure modes:
+ // 1. The two steps cannot be fenced: a concurrent thread
holds the same exchangeId (it read the
+ // same stored entry), so it would delete the winning
thread's recovery entry during its own
+ // cleanup on the compare-and-remove failure
(CAMEL-26796).
+ // 2. If the JVM stops or persistedCache.put() throws after
a successful cache.remove(), the group
+ // is in neither map (CAMEL-26663).
Review Comment:
🔧 **Nit:** Failure mode 2 describes a scenario (`persistedCache.put()`
throws after a successful `cache.remove()`) that belongs to the **reverse
(remove-first)** ordering, not the put-first ordering declared on line 368.
With put-first, `persistedCache.put()` is step 1 — it can't throw "after a
successful `cache.remove()`" because `cache.remove()` hasn't happened yet.
The comment correctly argues that *neither* ordering is safe without a
transaction, but conflating both orderings' failure modes under a single
"put-first" heading is misleading. Consider clarifying:
```suggestion
// A plain two-step approach (in either order) has two
failure modes:
// 1. put-then-remove (the pre-fix order): the two steps
cannot be fenced: a concurrent thread
// holds the same exchangeId (it read the same stored
entry), so it would delete the winning
// thread's recovery entry during its own cleanup on
the compare-and-remove failure (CAMEL-26796).
// 2. remove-then-put: if the JVM stops or
persistedCache.put() throws after a successful
// cache.remove(), the group is in neither map
(CAMEL-26663).
```
--
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]
