[fix](fe) Guard fragment cleanup until dispatch completes - #66767
Open
Mryange wants to merge 2 commits into
Open
[fix](fe) Guard fragment cleanup until dispatch completes#66767Mryange wants to merge 2 commits into
Mryange wants to merge 2 commits into
Conversation
### What problem does this PR solve? Issue Number: N/A Problem Summary: A load query could receive a FINISHED cleanup request while one backend was still waiting for its phase-two fragment start RPC. The backend then removed the Query Context before processing the start request, causing Failed to get query fragments context. Delay the FINISHED broadcast until query execution and all fragment dispatch RPCs have completed. ### Release note None ### Check List (For Author) - Test: sh run-fe-ut.sh --run org.apache.doris.qe.AbstractJobProcessorTest - Behavior changed: No - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Request changes.
The successful-dispatch handoff is directionally correct: the task is published before RPC submission, the two monotonic atomics cannot lose a wakeup, the final CAS prevents duplicate FINISHED fan-out, and the marker follows all applicable phase responses. Two blocking points remain: an expired-deadline exit can bypass backend cleanup after RPC submission, and the added tests do not exercise the production phase-two boundary.
Critical checkpoint conclusions
- Goal and correctness: successful one-phase and two-phase ordering now prevents premature FINISHED cleanup, but the pre-wait
leftTimeMs <= 0exit leaves the new success marker unreachable and does not invoke the real cancellation path. - Scope and parallel paths: the patch is small and focused. Modern query/load, one-/two-phase, ordinary/internal query, insert/TVF/broker load, cloud routing, and the legacy Coordinator path were traced. Legacy Coordinator is intentionally unaffected because it holds its dispatch lock through both phases and does not use this early top-fragment FINISHED handoff.
- Concurrency and lifecycle: report threads race the coordinator dispatch thread; AtomicBoolean ordering, task publication, the final CAS, and coordinator-to-backend-task lock order are sound, with no deadlock found. The expired-deadline branch is the remaining lifecycle defect because prepared or running BE query contexts can outlive FE failure/transaction abort until backend timeout.
- Error handling and data writes: per-future non-OK results, execution failures, interruption, and timeouts publish an error and synchronously cancel. Only the pre-loop deadline branch throws after future submission without equivalent cleanup. No other transaction, persistence, EditLog, or data-visibility change was found.
- Compatibility, configuration, and observability: no FE-BE wire field, storage format, persisted state, dynamic configuration, or rolling-upgrade change is introduced. Existing query/backend identifiers, latency profiles, failure metrics, and cancel logs are adequate; the added atomics are not a material hot-path cost.
- Tests: the new tests prove the two Boolean orders and idempotence, but they mock
PipelineExecutionTaskand call the marker directly, so they remain green if the sole production marker is deleted or moved before phase two. No builds or tests were run because the review bundle forbids them; current CI shows CheckStyle passing but does not expose an FE unit-test result. - User focus: no additional user-provided focus was supplied; the complete PR was reviewed.
Review completion: converged after two rounds. All normal and risk-focused reviewers in round 2 returned NO_NEW_VALUABLE_FINDINGS; every candidate was independently validated, deduplicated, accepted, or dismissed. The final inline set contains the two issues above.
| if (coordinatorContext.twoPhaseExecution()) { | ||
| sendAndWaitPhaseTwoRpc(); | ||
| } | ||
| coordinatorContext.getJobProcessor().markFragmentDispatchCompleted(); |
Contributor
There was a problem hiding this comment.
[P1] Cancel already-launched fragments when dispatch exits early
Both phase helpers launch all RPC futures before waitPipelineRpc checks leftTimeMs, but its leftTimeMs <= 0 branch throws without the updateStatusIfOk/cancelSchedule used by every per-future failure. For a load, a fast top fragment can report completion after one-phase submission or while phase-two starts are in flight; this patch records executionFinished, but this success-only marker is never reached. Load callers can then unregister or abort without cancelling the coordinator, leaving prepared or running BE query contexts until backend timeout. Please route the expired-deadline exit through the same real-error cancellation path before throwing and cover partial phase-one/phase-two dispatch in a test.
| processor.tryFinishSchedule(); | ||
| Mockito.verifyNoInteractions(fragmentsTask); | ||
|
|
||
| processor.markFragmentDispatchCompleted(); |
Contributor
There was a problem hiding this comment.
[P2] Exercise the production phase-two boundary
These tests call tryFinishSchedule() and markFragmentDispatchCompleted() directly while PipelineExecutionTask is only a mock, so they still pass if the sole production marker call is deleted, moved before phase two, or skipped by the load path. That means they do not reproduce the reported race. Please drive PipelineExecutionTask.execute() with controllable phase-two futures: report load completion while one start future is pending, assert no FINISHED cleanup, complete the final start response, and then assert exactly one cleanup.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Follow-up review: no new non-duplicate inline findings.
The current head addresses the previously reported dispatch-deadline cleanup defect: the pre-wait timeout now records a real coordinator error and cancels already-launched/prepared backend contexts before throwing. The existing production phase-two test concern remains applicable and unresolved, so I have not duplicated it.
Critical checkpoint conclusions
- Goal and correctness: the code now accomplishes the lifecycle goal.
PipelineExecutionTaskBuilderpublishes the task before RPC launch, the dispatch marker follows every applicable phase response, the two monotonic atomics cannot lose a crossing notification, and the final CAS permits exactly one FINISHED fan-out. The BE start response is produced only after its query context is found and made runnable. - Scope and clarity: the production change is small and focused: two completion signals, one interface method/call site, and cancellation in the previously uncovered pre-wait deadline branch. No unrelated source change was found.
- Concurrency: BE report RPC threads can race the coordinator dispatch thread. Both signal arrival orders, repeated reports, external/error cancellation, monitor reentrancy, and task-level cancellation serialization were traced. The atomics provide the needed visibility, coordinator-to-backend-task lock order is consistent, and no deadlock or duplicate broadcast was found.
- Lifecycle: success cleanup is withheld until all phase-one work and, when applicable, all phase-two start responses complete. Error/timeout paths retain first-error-wins coordinator state, release the load latch or query receiver, and cancel every backend task. No lost terminal wakeup or backend-context leak remains on a contractually reachable changed path.
- Configuration: no configuration item is added or changed; dynamic-reload behavior is not applicable.
- Compatibility: no function symbol, persisted representation, storage format, thrift/protobuf field, or FE-BE variable changes. Existing start/cancel RPCs are reused, so no rolling-upgrade compatibility fence is required.
- Parallel paths: SQL query, insert/load, broker load, one-phase and two-phase Nereids dispatch, report handling, external cancellation, and the legacy Coordinator contract were checked. Query completion already follows synchronous dispatch; load completion is the intended racing path. The legacy implementation is not changed and does not use this Nereids handoff.
- Conditional checks: the new two-flag condition directly represents the required invariant. The
leftTimeMs <= 0branch now matches the established per-future failure behavior by recording failure and cancelling before throwing. - Test coverage: the Boolean-order/idempotence tests and one-phase expired-deadline test cover their local mechanisms. They still do not drive
PipelineExecutionTask.execute()with a pending phase-two future and a concurrent load-completion report; that exact gap is already recorded in the existing thread linked above. - Test results: no build or test was run because the authoritative review bundle forbids builds. Current PR checks show CheckStyle passing, but no FE unit-test result is listed, so runtime test execution is not independently verified here.
- Observability: existing query/backend identifiers, timeout logs, RPC latency profile data, failure counters, blacklist handling, and cancel logs cover the changed paths. No additional metric or INFO log is needed for the two atomic state transitions.
- Transactions and persistence: no EditLog, replay, metadata persistence, transaction commit, or master-failover behavior is modified.
- Data writes: load transactionality and commit decisions remain outside this cleanup gate. A real dispatch error remains in FE status and releases/cancels the load path; a late FINISHED cleanup cannot convert it into a successful commit.
- FE-BE state propagation: no new state crosses the FE-BE boundary, and all existing dispatch/cancel send points remain intact.
- Performance: the change adds only constant-time atomic operations and a one-shot existing cleanup traversal; no meaningful hot-path CPU, memory, or allocation regression was found.
- Other issues: all ordinary exits after RPC launch either reach the success marker after full dispatch or invoke real cancellation. No additional correctness, lifecycle, compatibility, performance, or coverage issue could be substantiated beyond the existing test thread.
- User focus: no additional user-provided review focus was supplied; the complete PR was reviewed.
Review completion: converged after Round 1. Both normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS after missed-area rechecks. Every suspicious point was independently verified or mapped to existing review context. This is intentionally a comment-only review with zero new inline comments.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17458 ms |
Contributor
TPC-DS: Total hot run time: 85436 ms |
Contributor
ClickBench: Total hot run time: 14.42 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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
2 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.
Production stress-test logs captured the lifecycle race for a load query. At
13:35:52.506, BE received
FINISHEDand destroyed Query Context6aa1098800c457e-bad0398f81aa610f. About 61 ms later, its phase-twoexec_plan_fragment_startRPC arrived and failed withFailed to get query fragments context,causing the INSERT SELECT to fail. Root cause: FE could broadcast successful cleanup as soon as
execution completion was reported, without waiting for all fragment dispatch RPCs to finish. This
change delays
FINISHEDcleanup until both execution and fragment dispatch complete. It alsoensures that if the dispatch deadline has already expired after RPC futures were submitted, FE
records the error and actively cancels the coordinator so prepared or running backend contexts are
not left behind until backend timeout.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)