[bugfix](cancel) cancel message is printed too many times - #67344
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes for one cancellation/publication race.
- Goal and scope: Centralizing first-error cancellation in
QueryContextis a focused change that prevents repeated expensive PFC diagnostics and dependency fan-out for fragments already registered; the internal caller/signature migration is complete. - Concurrency and lifecycle: Scheduler, dependency wakeup, fragment prepare/publication, external cancellation, report, workload, and recursive rerun/close paths were traced. Map and task locks are released before cross-object cleanup, and no additional deadlock, UAF, or ownership cycle was found. The cancellation snapshot is not reconciled with later PFC publication, however; see the inline comment.
- Parallel and error paths: Registered fragments retain one local cleanup pass, first-error precedence, stream/load metadata handling, and report fallbacks. Submit, task close/finalize, report failure, workload cancellation, and both scheduler variants did not produce a distinct issue.
- Tests: No test changed. Existing injected-failure coverage proves only query failure, not once-only cleanup or the cancellation-versus-publication/early-
FINAL_CLOSEordering. The fix needs deterministic lifecycle coverage. - Compatibility, configuration, persistence, and data writes: No FE/BE protocol, rolling-upgrade, configuration, persisted-format, transaction, visibility, or storage-write change applies.
- Performance and observability: The intended one-time O(fragments plus tasks/dependencies) fan-out is preserved for registered PFCs. The unchanged per-task warning is a non-blocking residual behavior.
- User focus: No additional focus was provided.
Review completed for head 1cd7c5387ae53834bc24779266d86b826fb17bd7; one blocking inline issue remains.
| // Copy the fragment contexts under the map lock, then cancel them after releasing it. Fragment | ||
| // cancellation may take task-level locks and must not run while holding the query map lock. | ||
| std::vector<std::weak_ptr<PipelineFragmentContext>> ctx_to_cancel; | ||
| { |
Contributor
There was a problem hiding this comment.
[P1] Reconcile cancellation with late fragment publication
The query error is published before this snapshot, while set_pipeline_context() can insert afterward without observing the terminal status. A concrete recursive case is a cancel RPC arriving after the last task-prepare cancellation check but before FragmentMgr publishes a need_notify_close PFC: cancellation snapshots/removes the current query state, and the parent's one-shot FINAL_CLOSE can see NotFound; the prepare worker can then publish and submit the PFC. Its scheduler close path now re-enters QueryContext::cancel() and loses the first-error guard, so fragment-local cancel()/notify_close() never runs and _close_fragment_instance() cannot remove the PFC, leaving it and its QueryContext/rerunnable state strongly retained. Please linearize registration with cancellation (or reject publication into a terminal query), keep PFC cleanup outside the map lock, and cover this post-prepare race plus early FINAL_CLOSE deterministically.
Contributor
Author
There was a problem hiding this comment.
not a problem. And I will review this logic and fix
all the problems.
Contributor
Author
There was a problem hiding this comment.
not a problem because，maybe fix in another PR。
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 17021 ms |
Contributor
TPC-DS: Total hot run time: 83040 ms |
Contributor
ClickBench: Total hot run time: 14.8 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
When a query times out, every pending pipeline task can close with the same error and call PipelineFragmentContext::cancel() before the fragment task count is drained. Each call previously repeated fragment cancellation logs, the full timeout task dump, stream-pipe cancellation, and dependency wakeups, causing severe log amplification.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)