allthingssecurity commented on PR #28753: URL: https://github.com/apache/camel/pull/28753#issuecomment-6540636403
@davsclaus thanks for the review. Addressed in 0cfdccdbbfad (one commit on top, no force-push): 1. **Route ids after a CamelContext restart.** The route id is now also added in `onStart`, under `schedulerLock`. I added `testRestartedContextKeepsTheHalfOpenCheckOfTheRouteThatIsLeft`: two routes share the policy, `context.stop()`/`context.start()`, the circuit of the second route opens, the first route is removed, and the second route still gets its half-open check. However, I could not make the restart path fail without the change. The test also passes when the route id is added only in `onInit`. A probe that logs the callbacks on a `DefaultCamelContext` shows why: at `stop()`, `onRemove` runs for both routes; at `start()`, the routes are created again from their definitions (new `Route` objects), and `onInit` runs before `onStart`. So on camel-core the ids are filled again after a restart. I kept the `onStart` addition as a cheap guard and the test as a regression test, and corrected the code comment to say this. If you know of a context that restarts routes without creating them again, I'll add a test for it. Otherwise I can drop the `onStart` change if you'd rather not have it. 2. **`super.onInit(route)`.** `onInit` now calls it, so the `LoggingExceptionHandler` exists. `testFailureToSuspendTheConsumerIsLogged` uses a policy whose `suspendOrStopConsumer` throws, and checks with a log4j appender that the exception is logged at WARN under the policy's logger. It fails without the `super` call. Suggestions: - **`isReadyToBeClosed()` throwing.** The exception goes to `handleException`, and the circuit is reopened, so the next check runs after `halfOpenAfter`. This is caught where the handler is called, so it also covers the exchange-thread path. `HalfOpenTask.run` also catches any other exception and passes it to `handleException`, instead of leaving it in the `ScheduledFuture`. `testExceptionOfTheHalfOpenHandlerIsHandledAndTheCheckIsRetried`: the handler throws on the first check and is ready on the second. The exception reaches the exception handler and the second check closes the circuit. The test fails without the catch, and also when only `run` catches it (no retry). - **`halfOpenAfter <= 0`.** I chose rejection over a floor, because a floor would be a hidden magic value. `doInit` covers the constructor, and `setHalfOpenAfter` covers bean binding and JMX. Both throw `IllegalArgumentException`, so adding such a route fails. The 4.24 upgrade guide section now says this and gives the way back: a small value such as 1 to half open almost at once. `git grep` finds no use of 0 or less. `testHalfOpenAfterMustBeGreaterThanZero` covers it. - **`testRemovingOneOfTwoRoutesOfAPolicyKeepsItsHalfOpenCheck`.** It now uses `halfOpenAfter` 3000 ms. Tests: the `*Throttl*` classes and camel-test-dst passed (camel-core 80/0, camel-test-dst 59/0, camel-support 3/0). The two `Managed*` throttling tests pass when run alone. As before, they miscount the JVM-wide MBeans when run together with the other throttling classes. Each changed or new test class ran 20 times in one JVM with no failure. Negative controls, with the policy recompiled with one change: the follow-up's own tests fail against the previous policy, against the policy without `super.onInit`, against a rethrowing handler, and without the `doInit` check. The formatter changed nothing. The PR description is updated. _Claude Code on behalf of allthingssecurity_ -- 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]
