gnodet-bot commented on code in PR #26788:
URL: https://github.com/apache/camel/pull/26788#discussion_r4082911218
##########
core/camel-core-processor/src/main/java/org/apache/camel/processor/aggregate/AggregateProcessor.java:
##########
@@ -1915,6 +1960,8 @@ public int forceDiscardingOfGroup(String key) {
LOG.trace("Force discarded triggered for correlation key: {}",
key);
// force discarding by setting aggregate failed as true
onCompletion(key, exchange, exchange, false, true);
+ // the exchange is not submitted
+ unmarkCompleting(exchange.getExchangeId());
Review Comment:
**Nit: misleading comment + latent double-unmark (harmless but confusing)**
The comment says `"the exchange is not submitted"`, but that's only true
when `isDiscardOnAggregationFailure()` returns `false` (the default). In that
case `doOnCompletion` returns a non-null answer → the wrapper's `finally` block
does **not** call `unmarkCompleting` → the explicit call here is needed.
When `isDiscardOnAggregationFailure()` is `true`, `doOnCompletion` returns
`null` → the wrapper already calls `unmarkCompleting` → this call is redundant
(but safe, because `computeIfPresent` on an absent key is a no-op).
The same applies to the parallel site in `forceDiscardingOfAllGroups` (line
2005).
Consider a comment that explains the actual intent:
```suggestion
// the exchange was returned by onCompletion but not
forwarded to onSubmitCompletion,
// so unmarkCompleting was not called via that path — clear
the mark here
unmarkCompleting(exchange.getExchangeId());
```
##########
core/camel-core-processor/src/main/java/org/apache/camel/processor/aggregate/AggregateProcessor.java:
##########
@@ -1954,6 +2001,8 @@ public int forceDiscardingOfAllGroups() {
LOG.trace("Force discarded triggered for correlation
key: {}", key);
// force discarding by setting aggregate failed as true
onCompletion(key, exchange, exchange, false, true);
+ // the exchange is not submitted
+ unmarkCompleting(exchange.getExchangeId());
Review Comment:
Same misleading comment as in `forceDiscardingOfGroup` (see comment above).
The explicit `unmarkCompleting` is needed when
`isDiscardOnAggregationFailure=false` (default), where the wrapper's `finally`
does not unmark (because `answer != null`). When
`isDiscardOnAggregationFailure=true` the call is harmless but redundant.
```suggestion
// the exchange was returned by onCompletion but not
forwarded to onSubmitCompletion,
// so unmarkCompleting was not called via that path
— clear the mark here
unmarkCompleting(exchange.getExchangeId());
```
--
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]
