Repository navigation
br: Allow Skip Finegrained Scatter to Speed up Split & Scatter - #69175
Conversation
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
ti-chi-bot
Bot
added
release-note
|
Hi @YuJuncen. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIntroduces a two-phase region splitting strategy for snapshot restore. A coarse first pass splits and scatters a subset of keys sampled at configurable ChangesTwo-phase rough/fine region splitting with scatter control
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over CLI,RegionSplitter: Configuration wiring
CLI->>RestoreConfig: ParseFromFlags(FlagSplitRegionIndexStep, FlagCoarseScatter)
RestoreConfig->>RestoreConfig: Adjust: NormalizeRegionIndexStep
RestoreConfig->>SnapClient: SetSplitRegionIndexStep(step)
RestoreConfig->>SnapClient: SetCoarseScatter(coarseScatter)
end
rect rgba(144, 238, 144, 0.5)
Note over SnapClient,pdClient: Two-phase split execution
SnapClient->>RegionSplitter: NewRegionSplitterWithRegionIndexStep(splitClient, step)
RegionSplitter->>RegionSplitter: derive coarseKeys via regionIndexStep
RegionSplitter->>pdClient: SplitKeysAndScatter(coarseKeys) [phase 1, scatter=true]
pdClient-->>RegionSplitter: coarse regions scattered
RegionSplitter->>pdClient: SplitKeys or SplitKeysAndScatter(fullKeys) [phase 2, scatter=!coarseScatter]
pdClient-->>RegionSplitter: fine regions
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69175 +/- ##
================================================
- Coverage 76.3192% 76.1825% -0.1367%
================================================
Files 2041 2068 +27
Lines 561223 577136 +15913
================================================
+ Hits 428321 439677 +11356
- Misses 132000 135485 +3485
- Partials 902 1974 +1072
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
br/pkg/restore/split/mock_pd_client.go (1)
113-119: ⚡ Quick winDifferentiate scatter vs non-scatter behavior in these test doubles.
At Line 113/117 and Line 701/705, both method pairs behave identically, so tests using these clients can’t detect scatter-routing regressions. Please record/assert scatter intent separately (or simulate scatter side effects only in
SplitKeysAndScatter) to keep these fixtures contract-faithful.Also applies to: 701-707
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@br/pkg/restore/split/mock_pd_client.go` around lines 113 - 119, The SplitKeysAndScatter and SplitKeys methods in the mock client currently have identical implementations, both calling SplitWaitAndScatter with the same parameters, which prevents tests from detecting scatter-routing regressions. Differentiate these two methods by ensuring SplitKeysAndScatter simulates or records the scatter operation behavior while SplitKeys does not, either by passing different parameters to SplitWaitAndScatter or by calling different underlying methods. Apply this differentiation at both locations mentioned in the comment (around lines 113-119 and lines 701-707) to maintain consistent contract-faithful test fixtures that can distinguish between scatter and non-scatter routing paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@br/pkg/restore/split/mock_pd_client.go`:
- Around line 113-119: The SplitKeysAndScatter and SplitKeys methods in the mock
client currently have identical implementations, both calling
SplitWaitAndScatter with the same parameters, which prevents tests from
detecting scatter-routing regressions. Differentiate these two methods by
ensuring SplitKeysAndScatter simulates or records the scatter operation behavior
while SplitKeys does not, either by passing different parameters to
SplitWaitAndScatter or by calling different underlying methods. Apply this
differentiation at both locations mentioned in the comment (around lines 113-119
and lines 701-707) to maintain consistent contract-faithful test fixtures that
can distinguish between scatter and non-scatter routing paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5ae4a6bb-dab8-4241-b569-57a3129d5475
📒 Files selected for processing (5)
br/pkg/restore/split/client.gobr/pkg/restore/split/client_test.gobr/pkg/restore/split/mock_pd_client.gobr/pkg/restore/split/split.gobr/pkg/restore/split/split_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- br/pkg/restore/split/client_test.go
- br/pkg/restore/split/split.go
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
/retest-required |
|
@YuJuncen: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest-required |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Leavrth, RidRisR The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/ok-to-test |
3 participants
What problem does this PR solve?
Issue Number: close #69173
Problem Summary:
BR snapshot restore currently scatters both rough split regions and fine split regions. For restore workloads that only need coarse-grained region balancing, scattering every fine split region can add unnecessary PD scheduling work.
What changed and how does it work?
This PR adds two BR snapshot restore flags:
--coarse-scatter, defaulting tofalse.--split-region-index-step, a hidden flag defaulting to128, which controls the number of split-key indexes between two rough split keys.When
--coarse-scatter=true, snapshot restore still scatters the rough split regions, then performs the final fine split pass without scattering those fine split regions. The default restore behavior is unchanged.Both flags are passed through
RestoreConfigtoSnapClient, then intoRegionSplitter. The existingSplitKeysAndScatterbehavior remains unchanged, while the BR restore path can call the same split flow with scatter disabled for the final split pass.A playground verification record is included in
br_coarse_scatter_playground_verification.md.Check List
Tests
Unit test commands:
Manual test:
Then two database backup/restore runs were compared:
--split-region-index-step=2: BR restore log had 5scatter regionsentries.--split-region-index-step=2 --coarse-scatter=true: BR restore log had 1scatter regionsentry, beforefinish spliting regions roughly, and no fine split scatter entries afterward.See
br_coarse_scatter_playground_verification.mdfor the full commands and outputs.Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Release Notes
New Features
SplitRegionIndexSteptuning for restore rough-region splitting.CoarseScatterto control whether scattering is applied only during the rough-split phase.Tests