MPP: fix report status error when local RU is not initialized by windtalker · Pull Request #10963 · pingcap/tiflash

windtalker · 2026-07-08T09:18:50Z

What problem does this PR solve?

Issue Number: ref #10962

Problem Summary:

When TiFlash reports MPP task status with execution summaries enabled, ExecutorStatisticsCollector::fillLocalExecutionSummaries expects local RU consumption to be initialized. Some task paths can generate TiFlash execution info before MPPTaskStatistics::setRUInfo is called, so reporting status may fail with Check local_ru failed: local ru consumption info not setup and hide the original task status.

What is changed and how it works?

MPP: fix report status error when local RU is not initialized

Sync RU info into ExecutorStatisticsCollector before generating MPP execution summaries.
Initialize local RU consumption from the existing RU info source so missing RU info reports as zero instead of failing.
Add gtest coverage for generating MPP task execution info without calling setRUInfo first.

Check List

Tests

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

Added unit test: ExecutionSummaryTestRunner.genMPPTaskExecutionInfoWithoutSetRUInfo.

Side effects

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

Documentation

Release note

Fix an issue that TiFlash may fail to report MPP task status when local RU consumption has not been initialized.

Summary by CodeRabbit

Bug Fixes
- Ensured RU consumption is synchronized immediately before execution summary and TiFlash execution info serialization.
- Improved RU consumption handling in the statistics collector to initialize/clear optional values correctly.
- Prevented DeltaMerge segment read scheduling from queueing merged work during shutdown, reducing teardown-related issues.
Tests
- Added coverage for generating TiFlash MPP execution info without explicitly setting RU info (RU consumption present with zeroed fields).
- Updated segment read task tests to use failpoint-controlled stable behavior.
Chores
- Adjusted shutdown ordering to stop the segment read scheduler before tearing down reader resources.

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

coderabbitai · 2026-07-08T09:20:10Z

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: 6c995a73-0554-40fc-87fc-f239a35cb881

📥 Commits

Reviewing files that changed from the base of the PR and between 62a576c and 0165818.

📒 Files selected for processing (5)

dbms/src/Server/Server.cpp
dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp
dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.h
dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp
dbms/src/TestUtils/gtests_dbms_main.cpp

📝 Walkthrough

Walkthrough

RU consumption is now synced into execution responses before serialization, and SegmentReadTaskScheduler is stopped earlier during shutdown. The collector now reuses its RU optional in place, and tests cover RU output and scheduler-related teardown behavior.

Changes

RU consumption sync

Layer / File(s)	Summary
Sync helper and response call sites `dbms/src/Flash/Mpp/MPPTaskStatistics.h`, `dbms/src/Flash/Mpp/MPPTaskStatistics.cpp`	Adds `syncRUInfoToExecutorStatisticsCollector()`, routes `setRUInfo()` through it, and calls it before generating execution summary and TiFlash execution info.
Collector RU state and execution-summary test `dbms/src/Flash/Statistics/ExecutorStatisticsCollector.cpp`, `dbms/src/Flash/tests/gtest_execution_summary.cpp`	`setLocalRUConsumption()` now reuses the optional in place, and the new test parses RU consumption from execution summary output when RU info was not set.

SegmentReadTaskScheduler shutdown

Layer / File(s)	Summary
Stop state and task rejection `dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.h`, `dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp`	Adds `stop()`, renames the internal stop flag, makes destruction call `stop()`, and drops merged tasks once shutdown begins.
Shutdown wiring and test setup `dbms/src/Server/Server.cpp`, `dbms/src/TestUtils/gtests_dbms_main.cpp`, `dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp`	Stops the scheduler before reader-pool shutdown in server and gtest teardown, and enables the segment-update failpoint in the `DisaggReadSnapshot` stable setup.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MPPTaskStatistics
  participant ExecutorStatisticsCollector
  MPPTaskStatistics->>ExecutorStatisticsCollector: sync RU info
  MPPTaskStatistics->>MPPTaskStatistics: generate execution summary
  MPPTaskStatistics->>MPPTaskStatistics: generate TiFlash execution info

Possibly related issues

MPP: ReportMPPTaskStatus fails when local RU consumption is not initialized #10962: Addresses the same RU consumption reporting path in MPPTaskStatistics and ExecutorStatisticsCollector.

Possibly related PRs

pingcap/tiflash#10898: Shares the skip_check_segment_update failpoint setup in DisaggReadSnapshot test code.
pingcap/tiflash#10917: Modifies the same SegmentReadTaskScheduler shutdown flow and teardown ordering.

Suggested labels: release-note-none, size/M

Suggested reviewers: JaySon-Huang, Lloyd-Pottiger

Poem

A rabbit hopped by, soft and bright,
Syncing RU before the final write.
Then scheduler paws said, “stop, not yet,”
So teardown stayed tidy, neat, and set.
Hop-hop! 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main bug fix around reporting status when local RU is not initialized.
Description check	✅ Passed	The description covers the problem, fix, tests, and release note, matching the required template well enough.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.}

xzhangxian1008

lgtm

ti-chi-bot · 2026-07-08T09:46:15Z

[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:

~~OWNERS~~ [gengliqi]

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 · 2026-07-08T09:46:19Z

[LGTM Timeline notifier]

Timeline:

2026-07-08 09:40:50.563813113 +0000 UTC m=+188236.599908168: ☑️ agreed by gengliqi.
2026-07-08 09:46:17.11929242 +0000 UTC m=+188563.155387486: ☑️ agreed by xzhangxian1008.

windtalker · 2026-07-08T10:07:57Z

/test pull-integration-test

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker · 2026-07-08T18:26:18Z

/test pull-integration-test

windtalker · 2026-07-08T18:26:27Z

/test pull-unit-test

windtalker · 2026-07-09T00:22:32Z

/test pull-unit-test

windtalker · 2026-07-09T02:24:59Z

/test pull-unit-test

dillon-zheng · 2026-07-09T02:49:52Z

/retest

windtalker · 2026-07-09T05:28:18Z

/test pull-unit-test

…te+mergeDeltaAll (pingcap#10898) close pingcap#10897\n\nSigned-off-by: JaySon-Huang <tshent@qq.com>

…gmentReaderPoolManager shutdown (pingcap#10917) close pingcap#10903\n\nAdd a public `stop()` method to `SegmentReadTaskScheduler` that sets the stop flag and joins the background thread. Call it **before** `SegmentReaderPoolManager::stop()` in both production (`Server.cpp`) and test (`gtests_dbms_main.cpp`) shutdown paths. - `SegmentReadTaskScheduler.h`: expose `stop()` as public method; rename `std::atomic<bool> stop` → `stop_flag` to avoid name collision - `SegmentReadTaskScheduler.cpp`: implement `stop()`, destructor delegates to it - `Server.cpp`: call `SegmentReadTaskScheduler::instance().stop()` before `SegmentReaderPoolManager::instance().stop()` - `gtests_dbms_main.cpp`: same reorder Also fix two minor typos along the way: - `reapPeningPools` → `reapPendingPools` in class comment - `max_free_threds` → `max_free_threads` in test main

fix report status error

bce2e8a

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 8, 2026

gengliqi approved these changes Jul 8, 2026

View reviewed changes

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

xzhangxian1008 approved these changes Jul 8, 2026

View reviewed changes

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

fix ut

62a576c

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

JaySon-Huang added 2 commits July 9, 2026 13:35

test: fix flaky DisaggReadSnapshot — skip background tasks during wri…

e06949f

…te+mergeDeltaAll (pingcap#10898) close pingcap#10897\n\nSigned-off-by: JaySon-Huang <tshent@qq.com>

ti-chi-bot Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 9, 2026

ti-chi-bot Bot merged commit 4b2815a into pingcap:feature/release-8.5-materialized-view Jul 9, 2026
5 checks passed

windtalker mentioned this pull request Jul 9, 2026

Fix report status error #10972

Merged

12 tasks

Uh oh!

Conversation

windtalker commented Jul 8, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

xzhangxian1008 left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot Bot commented Jul 8, 2026

Uh oh!

ti-chi-bot Bot commented Jul 8, 2026

[LGTM Timeline notifier]

Uh oh!

windtalker commented Jul 8, 2026

Uh oh!

windtalker commented Jul 8, 2026

Uh oh!

windtalker commented Jul 8, 2026

Uh oh!

windtalker commented Jul 9, 2026

Uh oh!

windtalker commented Jul 9, 2026

Uh oh!

dillon-zheng commented Jul 9, 2026

Uh oh!

windtalker commented Jul 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

windtalker commented Jul 8, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 8, 2026 •

edited

Loading