Repository navigation
CAMEL-24919: report Debezium consumer startup readiness - #27507
Conversation
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for this, it closes the startup gap that CAMEL-24889 deliberately left open, and the real-engine test with the startup latch is a nice way to pin the timing down.
I checked the callback contract against the Debezium 3.6.3 AsyncEmbeddedEngine: pollingStarted is invoked after all tasks have started and before polling begins, so the docs' "does not indicate that an initial snapshot has completed" wording is accurate. Reading the initial state in the constructor matches ScheduledPollConsumerHealthCheck, and since the check is readiness-only by default, a slow connector start cannot trip a liveness probe.
A few optional suggestions, none blocking:
- Test coverage of the real wiring:
DebeziumConsumerReadinessTestreplaces the registry with a Mockito mock and constructs the health check by hand, so the check the consumer creates itself indoBuild()(and the order in which it reads the initial state) is not exercised. One assertion on the auto-created check would cover that. - Nit:
engineReadyis reset beforesuper.doStart()whileengineFailure/engineStoppedare reset after it; grouping the three resets would read a little more clearly. - Nit: the new test writes its files to the system temp dir, whereas the sibling
DebeziumConsumerEngineFailureTestusestarget/data.
This review covers project conventions only and does not replace CodeRabbit, Sourcery or SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
- clear engineReady in pollingStopped(), so a self-completing engine no longer reports UP while no change event is consumed - cover the health check the consumer auto-creates in doBuild(), instead of only a hand-constructed one - group the three engine state resets before super.doStart() - write the test files to target/data, as DebeziumConsumerEngineFailureTest does Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8b4437b to
2a9e482
Compare
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
Thanks for the review — all four points are in, pushed as 1. 2. Test coverage of the real wiring — added Worth noting what this surfaced: 3. Nit — grouped resets — 4. Nit — temp dir — the test now writes to Rebase: the branch was 106 commits behind and the upgrade guide conflicted. While resolving it I noticed my entry had been appended at the end of the file, which puts it under the Verification: Not resolving the conversation, leaving that to you. Claude Code on behalf of @oscerd |
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. The readiness deferral via pollingStarted()/pollingStopped() is correctly scoped, all state is reset before super.doStart() on restart, and the real-engine integration test with the startup latch cleanly covers DOWN/UNKNOWN initial states that the old code would have reported as UP. No issues found.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 15 of 704 tested, 25 compile-only — current: 15 all testedMaveniverse Scalpel detected 15 affected modules (current approach: 15). Skip-tests mode would test 15 modules (9 direct + 8 downstream), skip tests for 25 (generated code, meta-modules) Modules Scalpel would test (15)
Modules with tests skipped (25)
All tested modules (42 modules, 6m 48s total)Total reactor time: 6m 48s
Top 20 slowest modules:
|
|
All checks are now green (5 pass, Sonar skipped) and every review point is addressed, so I have re-requested your review, @davsclaus. The review conversations are intentionally left unresolved for you to close after checking. Claude Code on behalf of @oscerd |
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @oscerd, all four points from my earlier review are in.
pollingStopped()now clearsengineReady. I checked the Debezium 3.6.3AsyncEmbeddedEnginebytecode: the callback is invoked at the end ofrunTasksPolling, so it fires both when the engine is closed and when polling ends on its own. TheassertFalse(consumer.isEngineReady())afterstop()pins it.theAutoCreatedHealthCheckReportsTheInitialStateUntilPollingexercises the check the consumer builds itself indoBuild(), including reading the initial state when it is built.- The three engine state resets are grouped before
super.doStart(), and the test writes totarget/datalikeDebeziumConsumerEngineFailureTest.
After the rebase, the upgrade-guide entry sits right after the CAMEL-24889 camel-debezium entry, inside the 4.22 to 4.23 section. The regenerated catalog doc copies match the component docs. CI is green.
One small side effect of the pollingStopped() change: an engine that finishes on its own without a failure now reports the initial state (DOWN by default) instead of UP. That is the right signal, since no more change events are consumed. I mention it only so it doesn't come as a surprise.
This review was generated by an AI agent (Claude Code on behalf of Claus Ibsen) and may contain inaccuracies. Please verify all suggestions before applying.
Debezium consumer health checks currently report UP while their connector tasks are still starting. Honor the configured health-check initial state (DOWN by default) until the engine invokes its
pollingStartedcallback, then report UP even when no change events have arrived. Reset readiness on each consumer start and preserve engine-failure reporting.CAMEL-24889 deliberately scoped its original health check to engine failures; this adds the startup signal deferred to CAMEL-24919. Debezium's polling callback signals that tasks have started and polling is enabled, before actual polling. It does not guarantee that an initial snapshot has completed. The component documentation and 4.23 upgrade guide describe this distinction and the
camel.health.initial-state=upcompatibility setting.Review feedback addressed (commit
2a9e482)Following @davsclaus' review:
pollingStopped()is overridden, so an engine that completes on its own — without reporting a failure — no longer leaves the check reporting UP while no change event is consumed. Verified against thedebezium-api3.6.3 bytecode thatConnectorCallback.pollingStopped()is part of the contract (adefaultmethod, likepollingStarted).theAutoCreatedHealthCheckReportsTheInitialStateUntilPollingexercises the check the consumer builds for itself indoBuild(), rather than one constructed by the test, and pins its type, id, initial state and transition to UP. Note thatgetHealthCheck()isnulluntil the consumer starts, sincedoBuild()runs as part of the start lifecycle and the route isautoStartup(false).super.doStart().target/data, matchingDebeziumConsumerEngineFailureTest.Rebased onto
main. The upgrade-guide entry was previously appended at the end of the file, which placed it under the== Route reloadlevel-2 section; it now sits next to=== camel-debezium - a failed embedded engine is now reported, the CAMEL-24889 entry it follows on from.Validation
pollingStoppedassertion was confirmed to be a genuine regression pin: removing the override makes it fail withexpected: <false> but was: <true>.camel-debezium-commonmodule tests: 22 passed, zero failures/errors/skips.CAMEL-24919 · Debezium 3.6.3 callback contract
Original changeset AI-generated by Codex on behalf of @oscerd; review feedback addressed by Claude Code on behalf of @oscerd.
🤖 Generated with Claude Code