Skip to content

[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
yiguolei:refactor_fragmentmgr
Open

[refactor](fragment mgr) move report logic to pipeline fragment context to remove callback parameter from ctor#62500
yiguolei wants to merge 1 commit intoapache:masterfrom
yiguolei:refactor_fragmentmgr

Conversation

@yiguolei
Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary:

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen
Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@yiguolei
Copy link
Copy Markdown
Contributor Author

/review

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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 PipelineFragmentContext so 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 on FragmentMgr'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_report acquire/release CAS, async callbacks still run on the fragment manager thread pool, and no new lock nesting or heavy work under _task_mutex was introduced. The existing lifetime protection also remains intact because the submitted closure captures a shared ctx.
  • 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 PipelineFragmentContext alive until reporting completes.
  • Config changes: None.
  • Compatibility / storage format: None.
  • Parallel code paths: The two fragment creation paths in FragmentMgr (exec_plan_fragment and rerun_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.

@yiguolei
Copy link
Copy Markdown
Contributor Author

run buildall

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants