[refactor](fragment mgr) move report logic to pipeline fragment context to remove callback parameter from ctor#62500
Open
yiguolei wants to merge 1 commit intoapache:masterfrom
Open
[refactor](fragment mgr) move report logic to pipeline fragment context to remove callback parameter from ctor#62500yiguolei wants to merge 1 commit intoapache:masterfrom
yiguolei wants to merge 1 commit intoapache:masterfrom
Conversation
…xt to remove callback parameter from ctor
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
No blocking findings after reviewing the full reporting flow refactor from FragmentMgr into PipelineFragmentContext.
Critical checkpoints:
- Goal / correctness: The goal is to move FE report submission logic into
PipelineFragmentContextso callers no longer inject a report callback. The patch accomplishes that, and the end-to-end behavior remains equivalent:send_report()still submits async work onFragmentMgr's thread pool, attaches the query mem tracker in the callback, reports to FE, refreshes periodic-report state only after a non-final report completes, and preserves recursive-CTE final-close behavior. - Scope / clarity: The change is focused and mostly mechanical. The constructor surface is reduced and the reporting helpers now live with the state they operate on, which improves locality without broad behavioral churn.
- Concurrency: This path is concurrent, but the refactor preserves the existing model. Periodic reporting is still serialized by
_disable_period_reportacquire/release CAS, async callbacks still run on the fragment manager thread pool, and no new lock nesting or heavy work under_task_mutexwas introduced. The existing lifetime protection also remains intact because the submitted closure captures a sharedctx. - Lifecycle management: No new cross-TU static initialization hazards were introduced. Object lifetime remains safe for the moved callback because the async task keeps the
PipelineFragmentContextalive until reporting completes. - Config changes: None.
- Compatibility / storage format: None.
- Parallel code paths: The two fragment creation paths in
FragmentMgr(exec_plan_fragmentandrerun_fragment) were updated consistently. - Special conditions / invariants: The existing report gating rules for success, cancel, periodic reporting, and external queries are preserved.
- Tests: No new tests were added. For this refactor that is tolerable, but the reporting / rerun lifecycle remains an area where regression coverage would still be valuable.
- Test result files: Not applicable.
- Observability: Existing logging and debug hooks were preserved.
- Transaction / persistence: Not involved.
- Data writes / atomicity: Not involved beyond status reporting; no new atomicity risk found.
- FE/BE variable propagation: Not involved.
- Performance: Neutral to slightly positive. The patch removes an indirection through an injected callback and keeps the async execution model unchanged.
- Other issues: None identified that rise to blocking severity.
Overall opinion: This is a reasonable, focused refactor and I did not find a correctness or concurrency issue that should block the PR.
Contributor
Author
|
run buildall |
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
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
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)