Skip to content

Fix report status error#10972

Merged
ti-chi-bot[bot] merged 2 commits into
pingcap:masterfrom
windtalker:fix_report_status_error
Jul 9, 2026
Merged

Fix report status error#10972
ti-chi-bot[bot] merged 2 commits into
pingcap:masterfrom
windtalker:fix_report_status_error

Conversation

@windtalker

@windtalker windtalker commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #10962

Problem Summary:

port #10963 to master branch

What is changed and how it works?


Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

Summary by CodeRabbit

  • Bug Fixes

    • Execution summary and TiFlash execution info now include up-to-date resource usage data more reliably.
    • Resource consumption details are handled more consistently when they were not explicitly set, preventing missing or stale values in reported execution info.
  • Tests

    • Added coverage for execution summaries to verify resource usage is present and defaults to zero when no usage info is provided.

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
@ti-chi-bot ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 9, 2026
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 54347f93-4243-4cba-81a4-40b55738a380

📥 Commits

Reviewing files that changed from the base of the PR and between 316c706 and e1d20b8.

📒 Files selected for processing (4)
  • dbms/src/Flash/Mpp/MPPTaskStatistics.cpp
  • dbms/src/Flash/Mpp/MPPTaskStatistics.h
  • dbms/src/Flash/Statistics/ExecutorStatisticsCollector.cpp
  • dbms/src/Flash/tests/gtest_execution_summary.cpp

📝 Walkthrough

Walkthrough

This change fixes a crash where reporting MPP task status failed when local RU consumption was not initialized. RU consumption info is now synced into ExecutorStatisticsCollector before generating execution responses, and local_ru storage is reused instead of reallocated. A regression test validates the fix.

Changes

RU Consumption Sync Fix

Layer / File(s) Summary
Sync RU info before generating execution responses
dbms/src/Flash/Mpp/MPPTaskStatistics.h, dbms/src/Flash/Mpp/MPPTaskStatistics.cpp
A new private syncRUInfoToExecutorStatisticsCollector() method syncs RU info into executor_statistics_collector; it's called from setRUInfo and before both execution-info generation methods return, ensuring RU info is always synced.
Reuse local_ru storage on repeated calls
dbms/src/Flash/Statistics/ExecutorStatisticsCollector.cpp
setLocalRUConsumption now emplaces local_ru only if unset, otherwise clears it in place, avoiding unnecessary reallocation.
Regression test for missing RU info
dbms/src/Flash/tests/gtest_execution_summary.cpp
A new test generates TiFlash execution summaries without explicitly setting RU info and asserts the resulting RU consumption fields default to zero rather than causing an error.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MPPTask
  participant MPPTaskStatistics
  participant ExecutorStatisticsCollector
  MPPTask->>MPPTaskStatistics: genTiFlashExecutionInfo()/genExecutionSummaryResponse()
  MPPTaskStatistics->>MPPTaskStatistics: syncRUInfoToExecutorStatisticsCollector()
  MPPTaskStatistics->>ExecutorStatisticsCollector: setLocalRUConsumption(ru_info)
  ExecutorStatisticsCollector->>ExecutorStatisticsCollector: emplace or Clear local_ru
  ExecutorStatisticsCollector-->>MPPTask: execution response with RU consumption
Loading

Poem

A rabbit hops through RU and sync,
No more crashes make me blink!
local_ru now stays in place,
Cleared, not tossed, a tidy space.
Tests confirm the zeros hold—
ReportMPPTaskStatus, bold! 🐇✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and directly reflects the core fix for the report status failure.
Linked Issues check ✅ Passed The changes prevent RU-related exceptions during TiFlash execution info generation and add coverage for the uninitialized RU case.
Out of Scope Changes check ✅ Passed The code and test changes are focused on the reported MPP status failure and do not introduce unrelated scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Tools execution failed with the following error:

Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error)


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 9, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gengliqi, xzhangxian1008

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added lgtm approved and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 9, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

[LGTM Timeline notifier]

Timeline:

  • 2026-07-09 07:01:11.461544533 +0000 UTC m=+265057.497639619: ☑️ agreed by xzhangxian1008.
  • 2026-07-09 07:01:46.421696698 +0000 UTC m=+265092.457791754: ☑️ agreed by gengliqi.

@windtalker

Copy link
Copy Markdown
Contributor Author

/test pull-unit-test

@ti-chi-bot ti-chi-bot Bot merged commit b5093dd into pingcap:master Jul 9, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MPP: ReportMPPTaskStatus fails when local RU consumption is not initialized

3 participants