[CodeRabbit review] upstream #10417: Limit memory usage of exchange sender (#10387)

[jebter]

Review-only mirror of upstream PR pingcap#10417: pingcap#10417

The source and base branches are immutable review-only mirrors of the exact upstream head and base commits. Do not merge, close, modify, delete, force-push, or rebase this PR or its branches. The shared .coderabbit.yaml is review-enabling metadata only and is not an upstream code change.

Summary by CodeRabbit

• Performance

  • Improved streaming and data exchange buffering by honoring configured memory limits.
  • Buffering now considers both data size and row count, helping control memory usage across exchange operations.
  • Buffer limits are adjusted according to concurrent processing streams for more consistent resource usage.

• Reliability

  • Improved response flushing behavior for more predictable packet delivery across workload sizes.

• Tests

  • Added coverage for buffering behavior across streaming, shuffle, partition, and broadcast exchange scenarios.

[jebter]

@coderabbitai review

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: da72e22f-4034-4884-9857-c1b659bee399

📥 Commits

Reviewing files that changed from the base of the PR and between 642b17a and cc2e1b7.

📒 Files selected for processing (20)

• .coderabbit.yaml
• dbms/src/Flash/Coprocessor/DAGDriver.cpp
• dbms/src/Flash/Coprocessor/DAGResponseWriter.h
• dbms/src/Flash/Coprocessor/DAGUtils.cpp
• dbms/src/Flash/Coprocessor/DAGUtils.h
• dbms/src/Flash/Coprocessor/StreamingDAGResponseWriter.cpp
• dbms/src/Flash/Coprocessor/StreamingDAGResponseWriter.h
• dbms/src/Flash/Coprocessor/tests/gtest_streaming_writer.cpp
• dbms/src/Flash/Coprocessor/tests/gtest_ti_remote_block_inputstream.cpp
• dbms/src/Flash/Mpp/BroadcastOrPassThroughWriter.cpp
• dbms/src/Flash/Mpp/BroadcastOrPassThroughWriter.h
• dbms/src/Flash/Mpp/FineGrainedShuffleWriter.cpp
• dbms/src/Flash/Mpp/FineGrainedShuffleWriter.h
• dbms/src/Flash/Mpp/HashPartitionWriter.cpp
• dbms/src/Flash/Mpp/HashPartitionWriter.h
• dbms/src/Flash/Mpp/newMPPExchangeWriter.cpp
• dbms/src/Flash/Mpp/newMPPExchangeWriter.h
• dbms/src/Flash/Mpp/tests/gtest_mpp_exchange_writer.cpp
• dbms/src/Flash/Planner/Plans/PhysicalExchangeSender.cpp
• dbms/src/Storages/KVStore/tests/gtest_kvstore_fast_add_peer.cpp

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The change adds byte-based buffering limits to response and MPP exchange writers. It propagates limits from executor settings, adds flush checks and tests, enables CodeRabbit review configuration, and increases KVStore test polling attempts.

Changes

Buffered writer limits

Layer / File(s)	Summary
Buffering policy and shared state dbms/src/Flash/Coprocessor/DAGResponseWriter.h, dbms/src/Flash/Coprocessor/DAGUtils.*	Adds shared row and byte counters, flush checks, and a minimum-based per-concurrency buffer limit helper.
Writer buffering behavior dbms/src/Flash/Coprocessor/StreamingDAGResponseWriter.*, dbms/src/Flash/Mpp/*Writer.*	Updates response and exchange writers to track bytes and rows, flush at either limit, and reset counters after writes.
Exchange limit propagation dbms/src/Flash/Mpp/newMPPExchangeWriter.*, dbms/src/Flash/Planner/Plans/PhysicalExchangeSender.cpp, dbms/src/Flash/Coprocessor/DAGDriver.cpp	Computes limits from executor settings and concurrency, then passes them through exchange and DAG response writer construction.
Buffer-limit validation dbms/src/Flash/Coprocessor/tests/*, dbms/src/Flash/Mpp/tests/*	Tests small and large byte limits and updates writer construction calls with explicit limits.

Review configuration

Layer / File(s)	Summary
Auto-review branch configuration .coderabbit.yaml	Enables auto-review for matching coderabbit/release-8.5-base-* branches.

KVStore test retry

Layer / File(s)	Summary
Polling retry window dbms/src/Storages/KVStore/tests/gtest_kvstore_fast_add_peer.cpp	Increases polling attempts from 5 to 20 while retaining the 500 ms delay.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to cc2e1

This change adds bounded byte-based flushing for response and exchange writers, reducing buffering pressure while preserving row-based limits. The supplied coverage exercises the new limit behavior across writer types, and no merge-blocking risk is identified.

Sequence Diagram(s)

sequenceDiagram
  participant PhysicalExchangeSender
  participant newMPPExchangeWriter
  participant ExchangeWriter
  participant StorageStream
  PhysicalExchangeSender->>newMPPExchangeWriter: pass concurrency-adjusted buffer limit
  newMPPExchangeWriter->>ExchangeWriter: construct selected exchange writer
  StorageStream->>ExchangeWriter: write block
  ExchangeWriter->>ExchangeWriter: flush when row or byte limit is reached

Loading

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description explains that this is a review-only mirror and provides repository-specific handling instructions, but it omits the required problem summary, changes and implementation details, checkl…	Add the required template sections. Include the problem summary and issue number, describe the implementation changes, mark the applicable test and side-effect checklist items, document user or compatibility impacts, and provide the require…
Docstring Coverage	⚠️ Warning	Docstring coverage is 4.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 42 functions across 19 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: limiting memory usage of the exchange sender. The upstream PR and review metadata do not obscure the primary purpose.
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.

Full details: Description check

Explanation

The description explains that this is a review-only mirror and provides repository-specific handling instructions, but it omits the required problem summary, changes and implementation details, checklist, side effects, documentation, and release note sections from the template.

Resolution

Add the required template sections. Include the problem summary and issue number, describe the implementation changes, mark the applicable test and side-effect checklist items, document user or compatibility impacts, and provide the required release note entry.

Full details: Docstring Coverage

Explanation

Docstring coverage is 4.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 42 functions across 19 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch coderabbit/release-8.5-pr-10417-config

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[jebter]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.