Skip to content

fix: Retain exception that triggered Dart query cancellation.#19553

Merged
gianm merged 1 commit into
apache:masterfrom
gianm:dart-cancel-client-gone-away
Jun 4, 2026
Merged

fix: Retain exception that triggered Dart query cancellation.#19553
gianm merged 1 commit into
apache:masterfrom
gianm:dart-cancel-client-gone-away

Conversation

@gianm

@gianm gianm commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

In some cases, cancellation is triggered by an exception (rather than a non-exceptional reason, like timeout or user request). This patch retains the exception and includes it in the query report.

In some cases, cancellation is triggered by an exception (rather than a
non-exceptional reason, like timeout or user request). This patch
retains the exception and includes it in the query report.
@github-actions github-actions Bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Jun 4, 2026

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity Findings
P0 0
P1 0
P2 1
P3 0
Total 1

Reviewed 8 of 8 changed files.

Findings that could not be attached inline:

  • multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerHolder.java:249 - [P2] Preserve cancellation causes before the controller starts. cancel(reason, cause) only records cancelReason while the holder is still ACCEPTED. If the Dart result sequence closes or throws before the run executor starts the controller, the later runAsync path skips controller.stop(...) and builds makeCanceledReport(cancelReason), so the exception passed from DartQueryMaker is dropped and the report is still a bare UNKNOWN cancellation. Store the cause in the holder and use it when building the pre-run canceled report.

This is an automated review by Codex GPT-5.5

@capistrant capistrant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Franks auto review comment looks accurate, but it feels like an improvement that could be a follow if you don't want to work on implementing the idea now

@gianm gianm merged commit 9d82a0e into apache:master Jun 4, 2026
138 of 142 checks passed
@gianm gianm deleted the dart-cancel-client-gone-away branch June 4, 2026 21:32
@github-actions github-actions Bot added this to the 38.0.0 milestone Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants