Croway commented on PR #27576:
URL: https://github.com/apache/camel/pull/27576#issuecomment-6061209267
AI-generated review by Codex on behalf of @Croway, reviewing head
`803c2cacf32f4ed1c7e353de065505f1941ab724`.
I found one reproduced statistics regression and one documentation mismatch.
1. **[P2] Preserve handled-failure statistics when suppressing nested
exchange counts.** In `ManagedRouteGroup.completedExchange()` (lines 226–228),
the guard skips the entire superclass update. That update also records
`FailuresHandled` and `LastExchangeFailureHandledTimestamp`.
A route in group `g` that wireTaps another route in `g` can complete
successfully while the tapped route throws and handles an exception. The
wireTap wrapper places the originating route on the tapped exchange's route
stack, so the guard suppresses the target's group update. The target route
reports `FailuresHandled == 1`, but the group reports `0`; its handled-failure
timestamp is not updated and remains null for a fresh group. The originating
exchange never receives the tapped exchange's handled-failure state, so its
group update cannot recover this information.
I verified the regression with the test patch below: it passes when
`ManagedRouteGroup.java` is replaced by the implementation from this PR's
parent, and fails with this PR's implementation with `expected: <1> but was:
<0>`. The failure repeated on all three configured test attempts.
Exchange-count deduplication should preserve handled-failure counts and
timestamps from group members, which CAMEL-24590 deliberately made visible.
2. **Documentation: qualify the once-per-exchange claim.**
`route-group.adoc` lines 180–181 and the upgrade notes say exchanges are
counted once per group. The guard only suppresses nested same-group routes. For
`outside[no group] -> a[g]`, followed by `outside -> b[g]`, the same exchange
still contributes two completions to `g`, while the CamelContext contributes
one. `ManagedRouteGroupTest` lines 63–64 explicitly expects this. Please
describe the outermost-route behavior and sequential-entry limitation, or
implement the stronger guarantee if that is intended.
Validation: the unchanged PR's 8 focused management tests passed, and all 7
new predicate-based route-stack tests passed. The additional regression test
below fails only on its group handled-failure assertion with the PR
implementation.
<details>
<summary>Regression test patch for
ManagedRouteGroupExchangeCountTest.java</summary>
Apply this patch to the PR head:
```diff
diff --git
a/core/camel-management/src/test/java/org/apache/camel/management/ManagedRouteGroupExchangeCountTest.java
b/core/camel-management/src/test/java/org/apache/camel/management/ManagedRouteGroupExchangeCountTest.java
index 9e8fb0492e95..f91daec622e2 100644
---
a/core/camel-management/src/test/java/org/apache/camel/management/ManagedRouteGroupExchangeCountTest.java
+++
b/core/camel-management/src/test/java/org/apache/camel/management/ManagedRouteGroupExchangeCountTest.java
@@ -146,6 +146,12 @@ public class ManagedRouteGroupExchangeCountTest extends
ManagementTestSupport {
return new RouteBuilder() {
@Override
public void configure() {
+
from("direct:review-wt").routeId("review-wt").routeGroup("review-failure")
+ .wireTap("direct:review-wt-target");
+
from("direct:review-wt-target").routeId("review-wt-target").routeGroup("review-failure")
+
.onException(IllegalStateException.class).handled(true).end()
+ .throwException(new IllegalStateException("Handled
tap failure"));
+
// x1 -> x2, both in group "g"
from("direct:x1").routeId("x1").routeGroup("g")
.to("direct:x2");
@@ -183,4 +189,14 @@ public class ManagedRouteGroupExchangeCountTest extends
ManagementTestSupport {
};
}
+ @Test
+ public void testReviewWireTapHandledFailureIsAggregated() throws
Exception {
+ ManagedCamelContext mcc =
context.getCamelContextExtension().getContextPlugin(ManagedCamelContext.class);
+ template.sendBody("direct:review-wt", "Hello");
+ await().atMost(5, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertEquals(1,
mcc.getManagedRoute("review-wt-target").getFailuresHandled()));
+ assertEquals(1,
mcc.getManagedRouteGroup("review-failure").getFailuresHandled());
+
assertNotNull(mcc.getManagedRouteGroup("review-failure").getLastExchangeFailureHandledTimestamp());
+ }
+
}
```
Run from the repository root:
```sh
mvn -pl core/camel-management -am \
'-Dtest=ManagedRouteGroupExchangeCountTest#testReviewWireTapHandledFailureIsAggregated'
\
-Dsurefire.failIfNoSpecifiedTests=false test
```
For the baseline comparison, I kept the same test and restored only
`ManagedRouteGroup.java` from the PR parent; that run passed, including the
non-null timestamp assertion.
</details>
--
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]
