fix: publish final Dart reports before result completion - #20293
FrankChen021 wants to merge 12 commits into
Conversation
github-actions
Bot
added
Area - Batch Ingestion
Area - MSQ
There was a problem hiding this comment.
🟢 Approval recommended
The concurrency and publication-order changes are covered by targeted regression tests, with only a minor test maintainability nit identified.
Pull request overview
This PR improves Dart MSQ report publication timing and registry atomicity so that final controller reports are visible immediately when query results complete, without a transient “stale counters” or “missing report” window during deregistration/retention.
Changes:
- Publish a finalized
TaskReport.ReportMapfromControllerHolderbefore invoking the completion listener, and serve it via the registry while the controller remains registered. - Make controller removal and completed-report retention atomic with completed-report lookups by synchronizing these operations on the same lock.
- Add regression tests covering visibility of the final report during the completion callback and lookup behavior during concurrent deregistration.
File summaries
| File | Description |
|---|---|
| multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerHolder.java | Publishes and serves a final report snapshot prior to completion callback execution. |
| multi-stage-query/src/main/java/org/apache/druid/msq/dart/controller/DartControllerRegistry.java | Ensures atomic deregistration/report retention relative to completed-report lookups; serves final reports while still registered. |
| multi-stage-query/src/test/java/org/apache/druid/msq/exec/ControllerHolderTest.java | Adds regression coverage for final-report visibility during completion listener execution. |
| multi-stage-query/src/test/java/org/apache/druid/msq/dart/controller/DartControllerRegistryTest.java | Adds concurrency test ensuring lookups wait for report publication during deregistration. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
Review deferred: GitHub currently reports this PR as conflicting with master.
Current head: 063eab0c2b20b8e5a37ade204696083916f7a867 (codex/fix-dart-report-publication). Please resolve the conflicts with master and push the updated head so the changes can be reviewed later.
Reviewed 0 of 0 changed files; review was deferred before preparation.
This is an automated review by Codex GPT-5.6-Luna(max)
…rt-publication # Conflicts: # indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, security, and integration risks; no issues found.
Reviewed 4 of 4 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, security, and integration risks; no issues found.
Reviewed 4 of 4 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
The normal completion path is correct: ControllerImpl publishes a final report through an AtomicReference before delegating completion, and registry removal/retention is serialized with completed-report reads. One lifecycle gap remains for cancellation before the controller thread starts: the synthetic canceled report is not holder-visible while the completion callback runs, and the holder monitor prevents the fallback lookup from acquiring the state it needs. This can leave the report unavailable for the duration of a slow or blocked callback.
Reviewed 7 of 7 changed files: all 5 production files and both modified test files, starting with the incremental diff and then rechecking the full current diff, surrounding lifecycle code, registry lookup/retention paths, all Controller implementations and call sites, and the prior reviewed head. No prior actionable MergeLens findings remained open.
Validation: static inspection only; no build or test commands were run.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
The report-publication and registry-transfer changes are consistent across the current head. The combined controller accessor returns the final report once it is atomically published and otherwise returns the live snapshot; pre-run cancellation now exposes a synthetic report without holding the holder monitor during the completion callback; and registry removal/retention is serialized with completed-report lookups. I found no concrete correctness, lifecycle, concurrency, security, data-loss, API-compatibility, or missing-test issue requiring action. The prior pre-run cancellation finding is resolved in this head.
Reviewed 9 of 9 changed files:
- multi-stage-query/src/main/java/org/apache/druid/msq/dart/controller/DartControllerRegistry.java
- multi-stage-query/src/main/java/org/apache/druid/msq/exec/CaptureReportQueryListener.java
- multi-stage-query/src/main/java/org/apache/druid/msq/exec/Controller.java
- multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerHolder.java
- multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java
- multi-stage-query/src/main/java/org/apache/druid/msq/rpc/ControllerResource.java
- multi-stage-query/src/test/java/org/apache/druid/msq/dart/controller/DartControllerRegistryTest.java
- multi-stage-query/src/test/java/org/apache/druid/msq/exec/ControllerHolderTest.java
- multi-stage-query/src/test/java/org/apache/druid/msq/indexing/client/ControllerChatHandlerTest.java
Validation: git diff --check 33f7f77..eed7dd2 passed; no build, test, dependency-install, or formatter commands were run.
This is an automated review by Codex GPT-5.6-Luna(max)
Description
Publish the final controller report before notifying the completion listener, and serve it while the controller is still registered. Keep removal of the active entry and retention of its report under the same lock used by completed-report lookups.
This closes two report-publication windows: stale live counters after result completion, and a missing report during registry transfer. PR #20291 CI observed missing stage-zero counters immediately after a successful Dart query. The integration test is unchanged.
Validation
Release note
Make final Dart query reports available when query results finish, without a temporary missing-report window during retention.
Review