allthingssecurity commented on PR #26989:
URL: https://github.com/apache/camel/pull/26989#issuecomment-5882546246
Checked fix 3 against a leak we had found on our side (the repositories keep
every `Route`/`Consumer` they have seen). The `retainAll` covers all three ways
a key goes stale:
- `removeRoute`;
- a route reload, which adds a new `Route` object (`DefaultRoute` uses
identity `equals`);
- a stop/start, which gives the route a new `Consumer`.
The test covers removal only. A stop/start loop would pin the consumer case:
on this branch, 5 × `stopRoute`/`startRoute` with `consumers.stream()` after
each leaves 1 entry. Before the fix, each restart added one.
One point about fix 1 that may need a decision: a check is also `UNKNOWN`
when the user disabled it. `healthCheckConsumerEnabled=false` on a component
makes `ScheduledPollConsumerHealthCheck` return `UNKNOWN` ("Disabled"), and
`ConsumerHealthCheck` copies that state. With this PR the application is then
never ready, at any exposure level. On this branch (a `scheduler:` route,
`healthCheckConsumerEnabled=false`):
```
full ready=false -> context=UP route:foo=UP consumer:foo=UNKNOWN(Disabled)
default ready=false -> consumer:foo=UNKNOWN(Disabled)
oneline ready=false -> consumer:foo=UNKNOWN(Disabled)
```
Before, `default` and `oneline` were ready and only `full` was not. The
levels now agree, but on "not ready", and turning a check off makes readiness
fail instead of ignoring the check. Would it be better to leave disabled checks
out of readiness (the result carries `check.enabled=false` in its details) and
keep the other `UNKNOWN` results as not ready? The upgrade-guide text would
then say how disabled checks are treated.
_Review by 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]
