Repository navigation
br: set scan region limiter (#66822) - #67142
Merged
ti-chi-bot[bot] merged 5 commits intoJun 3, 2026
Merged
ti-chi-bot[bot] merged 5 commits into
ti-chi-bot[bot] merged 5 commits into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
Member
Author
|
@Leavrth This PR has conflicts, I have hold it. |
2 of 13 tasks
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
📝 WalkthroughWalkthroughAdds a configurable region-scan concurrency limiter wired through restore clients and snap importer; enforces the limit with token-based flow control in snap importer and extends tests/scripts to validate and accommodate related behavior and test harness changes. ChangesRegion Scan Concurrency & Tests
GCS credential & endpoint script updates
Sequence Diagram(s)sequenceDiagram
participant Test as TestHarness
participant Importer as SnapFileImporter
participant PDMock as flowControlSplitClient
Test->>Importer: start 200 concurrent PaginateScanRegionForTest
Importer->>PDMock: ScanRegions requests (concurrent)
PDMock-->>Importer: region info (enforcing maxFlight)
Importer-->>Test: return results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
br/pkg/restore/snap_client/export_test.go (1)
164-177: Delegate this test helper to the real constructor.This is now a second manual initializer for
SnapFileImporterOptions. ReusingNewSnapFileImporterOptions(..., 0, nil, nil)here would keep future option defaults in sync and avoid another drift point when the struct grows.♻️ Suggested cleanup
func NewSnapFileImporterOptionsForTest( splitClient split.SplitClient, importClient importclient.ImporterClient, tikvStores []*metapb.Store, rewriteMode RewriteMode, concurrencyPerStore uint, ) *SnapFileImporterOptions { - return &SnapFileImporterOptions{ - metaClient: splitClient, - importClient: importClient, - tikvStores: tikvStores, - rewriteMode: rewriteMode, - concurrencyPerStore: concurrencyPerStore, - } + return NewSnapFileImporterOptions( + nil, + splitClient, + importClient, + nil, + rewriteMode, + tikvStores, + concurrencyPerStore, + 0, + nil, + nil, + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/pkg/restore/snap_client/export_test.go` around lines 164 - 177, The NewSnapFileImporterOptionsForTest function duplicates the SnapFileImporterOptions initializer; replace the manual struct literal with a call to the canonical constructor NewSnapFileImporterOptions and pass the test-specific values plus the defaults (e.g. 0, nil, nil) so defaults stay in sync with the real constructor; update NewSnapFileImporterOptionsForTest to delegate to NewSnapFileImporterOptions(splitClient, importClient, tikvStores, rewriteMode, 0, nil, nil) and remove the inline struct literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@br/pkg/restore/snap_client/import_test.go`:
- Around line 224-233: The test currently calls require.LessOrEqual and
require.NoError from a worker goroutine inside
flowControlSplitClient.ScanRegions (and the other worker at lines ~264-269);
move those assertions out of spawned goroutines by returning error/status values
to the caller via channels (e.g. an error channel or result struct channel)
instead of invoking require.* in the goroutine, and perform
require.LessOrEqual/require.NoError in the main test goroutine after receiving
from the channel; specifically modify ScanRegions and the other worker closure
to send any violation or error back, then assert on that received value in the
test harness.
---
Nitpick comments:
In `@br/pkg/restore/snap_client/export_test.go`:
- Around line 164-177: The NewSnapFileImporterOptionsForTest function duplicates
the SnapFileImporterOptions initializer; replace the manual struct literal with
a call to the canonical constructor NewSnapFileImporterOptions and pass the
test-specific values plus the defaults (e.g. 0, nil, nil) so defaults stay in
sync with the real constructor; update NewSnapFileImporterOptionsForTest to
delegate to NewSnapFileImporterOptions(splitClient, importClient, tikvStores,
rewriteMode, 0, nil, nil) and remove the inline struct literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e6041ffc-f142-4683-9927-04a017abb594
📒 Files selected for processing (12)
br/pkg/restore/log_client/client.gobr/pkg/restore/snap_client/BUILD.bazelbr/pkg/restore/snap_client/client.gobr/pkg/restore/snap_client/export_test.gobr/pkg/restore/snap_client/import.gobr/pkg/restore/snap_client/import_test.gobr/pkg/task/common_test.gobr/pkg/task/config_test.gobr/pkg/task/restore.gobr/pkg/task/stream.gobr/pkg/utils/worker.gobr/tests/br_pitr_table_filter/run.sh
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #67142 +/- ##
================================================
Coverage ? 55.7259%
================================================
Files ? 1824
Lines ? 657879
Branches ? 0
================================================
Hits ? 366609
Misses ? 264201
Partials ? 27069
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
YuJuncen
approved these changes
Jun 1, 2026
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
RidRisR
approved these changes
Jun 2, 2026
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
Contributor
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
Contributor
|
/retest |
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
ti-chi-bot
Bot
removed
the
size/L
ti-chi-bot
Bot
added
size/XL
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RidRisR, YuJuncen 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@br/tests/br_gcs/run.sh`:
- Line 111: The restore command invocation contains an unnecessary trailing '?'
in the storage URL ("gcs://$BUCKET/$DB?") which should be removed to avoid
confusing URL parsing; update the run_br call that executes "run_br restore full
-s \"gcs://$BUCKET/$DB?\" --pd $PD_ADDR --gcs.endpoint=...
--check-requirements=false" to use "gcs://$BUCKET/$DB" (no '?') while keeping
the --gcs.endpoint flag unchanged so the endpoint is still provided via
--gcs.endpoint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d7073377-21dc-49c3-87d9-31a8dd6fd795
📒 Files selected for processing (4)
br/compatibility/credentials/application_default_credentials.jsonbr/tests/br_gcs/run.shbr/tests/docker_compatible_gcs/_run.shbr/tests/docker_compatible_gcs/prepare.sh
✅ Files skipped from review due to trivial changes (1)
- br/tests/docker_compatible_gcs/prepare.sh
Contributor
|
/retest |
2 similar comments
Contributor
|
/retest |
Contributor
|
/retest |
This was referenced Jun 8, 2026
GMHDBJD
added a commit
to GMHDBJD/tidb
that referenced
this pull request
Oct 15, 2026
Backport only br/tests/br_gcs/run.sh from 0f820cb (pingcap#67142). Include the missing fake credential fields, current TiKV endpoint format, and upstream opt-in legacy BR coverage. Do not include the source commit production changes.
Merged
1 of 13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated cherry-pick of #66822
What problem does this PR solve?
Issue Number: close #66821 ref #67623
Problem Summary:
When there are many scan region requests sent to PD, PD will be stuck.
What changed and how does it work?
set region scan limiter to avoid too many scan region requests sent to PD at the same time.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Chores
Tests