Repository navigation
br: set scan region limiter - #66822
Conversation
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note
|
Review Complete Findings: 2 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/L
|
Hi @Leavrth. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a region-scan-concurrency knob, wires it through RestoreConfig, Restore/Log clients and SnapClient, implements token-based throttling in SnapFileImporter for PD PaginateScanRegion requests, updates tests, worker token utility bounds, and a BUILD test shard/dependency tweak. Changes
Sequence DiagramsequenceDiagram
participant User as CLI/User
participant RestoreCfg as RestoreConfig
participant SnapClient as SnapClient
participant Importer as SnapFileImporter
participant TokenCh as Token Channel
participant PD as PD (SplitClient)
User->>RestoreCfg: set region-scan-concurrency
RestoreCfg->>SnapClient: SetRegionScanConcurrency(value)
SnapClient->>Importer: NewSnapFileImporterOptions(..., scanConcurrency)
Importer->>TokenCh: BuildWorkerTokenChannel(scanConcurrency)
Note over Importer,TokenCh: During Import...
Importer->>Importer: paginateScanRegion()
Importer->>TokenCh: acquirePDReqToken()
TokenCh-->>Importer: token (or wait)
Importer->>PD: PaginateScanRegion(ctx,startKey,endKey)
PD-->>Importer: RegionInfo results
Importer->>TokenCh: releasePDReqToken()
TokenCh-->>Importer: token released
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Command failed 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 |
|
/ok-to-test |
![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
🤖 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-245: The test currently performs testify/require assertions
inside the mock ScanRegions and worker goroutines (see
flowControlSplitClient.ScanRegions, its inFlight/maxFlight use, and the worker
closure), which can call FailNow in a goroutine and not stop the main test;
instead, make ScanRegions block briefly (use a channel handoff) to actually
create concurrent in-flight calls, record any violations (e.g., cur > maxFlight)
to a thread-safe counter or slice using atomic ops or a mutex, and remove all
require/require.LessOrEqual calls from goroutines; after wg.Wait() in the main
test goroutine, assert once using require on the collected violations to ensure
failures are observed reliably.
In `@br/pkg/task/restore.go`:
- Around line 480-483: The code currently allows an explicit
--region-scan-concurrency=0 to be silently overwritten by Adjust(), so update
the flag handling after reading cfg.RegionScanConcurrency to detect whether the
flag was explicitly set: use flags.Changed(FlagRegionScanConcurrency) and if it
is true and cfg.RegionScanConcurrency == 0 return a clear invalid-argument error
(annotate with the flag name, e.g., in the same style as existing returns)
instead of letting Adjust() replace it; apply the same check where
cfg.RegionScanConcurrency is read at the later occurrence around the 613–615
region.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a999dd8e-df59-4a86-b9cf-bebc2362e309
📒 Files selected for processing (8)
br/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/config_test.gobr/pkg/task/restore.gobr/pkg/utils/worker.go
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66822 +/- ##
================================================
+ Coverage 77.6662% 79.0279% +1.3616%
================================================
Files 2008 1954 -54
Lines 549877 552863 +2986
================================================
+ Hits 427069 436916 +9847
+ Misses 121100 114882 -6218
+ Partials 1708 1065 -643
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>
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
br/pkg/utils/worker.go (1)
13-16: Add doc comments to the new exported limits.These are new exported symbols, and the
30Mcap is a non-obvious operational constraint that should be documented at the declaration.Possible patch
const ( + // DefaultWorkerTokenChannelSize is the fallback token-pool size when callers + // request a worker channel but pass 0. DefaultWorkerTokenChannelSize = 128 + // MaxWorkerTokenChannelSize caps caller-provided token-pool sizes to avoid + // pathological channel capacities and startup cost. MaxWorkerTokenChannelSize = 30 * 1024 * 1024 // 30M )As per coding guidelines, "Keep exported-symbol doc comments, and prefer semantic constraints over name restatement" and "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/pkg/utils/worker.go` around lines 13 - 16, Add clear exported doc comments for DefaultWorkerTokenChannelSize and MaxWorkerTokenChannelSize: describe what each constant controls (the default and maximum size of the worker token channel), the units (number of tokens), the reason/operational constraint behind the 30M cap (memory/GC/throughput trade-off and why larger values are unsafe), any concurrency or performance implications, and guidance for when it is safe to change the limit; attach these comments immediately above the constant declarations for DefaultWorkerTokenChannelSize and MaxWorkerTokenChannelSize.
🤖 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/task/restore.go`:
- Around line 262-267: RegionScanConcurrency is a plain uint so zero cannot
distinguish “unset” vs deliberate 0=unlimited, allowing zero-value configs to
bypass throttling; modify the config handling so RegionScanConcurrency has an
explicit unset sentinel (e.g. pointer *uint or use a boolean flag) and update
Adjust() to set the CLI default (256) only when unset, then ensure
configureRestoreClient and br/pkg/restore/snap_client/import.go check the
sentinel rather than raw 0 before deciding to create the pdReqTokens channel;
update any other uses (e.g., referenced lines ~480 and ~753) to respect the new
unset vs zero semantics.
---
Nitpick comments:
In `@br/pkg/utils/worker.go`:
- Around line 13-16: Add clear exported doc comments for
DefaultWorkerTokenChannelSize and MaxWorkerTokenChannelSize: describe what each
constant controls (the default and maximum size of the worker token channel),
the units (number of tokens), the reason/operational constraint behind the 30M
cap (memory/GC/throughput trade-off and why larger values are unsafe), any
concurrency or performance implications, and guidance for when it is safe to
change the limit; attach these comments immediately above the constant
declarations for DefaultWorkerTokenChannelSize and MaxWorkerTokenChannelSize.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 18339ac4-42fe-472a-9f0c-6e44a616ea8a
📒 Files selected for processing (3)
br/pkg/restore/snap_client/import.gobr/pkg/task/restore.gobr/pkg/utils/worker.go
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
/retest |
|
[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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
br/tests/br_pitr_table_filter/run.sh (1)
333-333: Avoid always-on-vvvin this test flow.Line 333 forces max SQL verbosity on every run, which can bloat CI logs and make failures harder to triage. Prefer opt-in verbosity via env flag.
♻️ Suggested adjustment
- run_sql_file $CUR/sqls/log.sql -vvv + if [ "${BR_TEST_VERBOSE_SQL:-0}" = "1" ]; then + run_sql_file "$CUR/sqls/log.sql" -vvv + else + run_sql_file "$CUR/sqls/log.sql" + fiAs per coding guidelines, "Keep changes focused; avoid unrelated refactors, renames, or moves in the same PR".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/tests/br_pitr_table_filter/run.sh` at line 333, Replace the hard-coded "-vvv" in the run_sql_file invocation so verbosity is opt-in: detect an environment flag (e.g. VERBOSE_SQL) and only append "-vvv" to the run_sql_file $CUR/sqls/log.sql call when that flag is set; update the call site that invokes run_sql_file to conditionally include the verbosity flag rather than always passing "-vvv" (reference the run_sql_file invocation and $CUR/sqls/log.sql).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@br/tests/br_pitr_table_filter/run.sh`:
- Line 333: Replace the hard-coded "-vvv" in the run_sql_file invocation so
verbosity is opt-in: detect an environment flag (e.g. VERBOSE_SQL) and only
append "-vvv" to the run_sql_file $CUR/sqls/log.sql call when that flag is set;
update the call site that invokes run_sql_file to conditionally include the
verbosity flag rather than always passing "-vvv" (reference the run_sql_file
invocation and $CUR/sqls/log.sql).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8cb05b6e-224c-449c-9c9a-1003f2d6c613
📒 Files selected for processing (2)
br/pkg/task/common_test.gobr/tests/br_pitr_table_filter/run.sh
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
Code reviewFound 1 issue:
Lines 464 to 490 in efd2f33 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
|
/retest |
1 similar comment
|
/retest |
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
|
/retest |
6 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
@Leavrth: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Leavrth
added
the
needs-cherry-pick-release-8.5
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
What problem does this PR solve?
Issue Number: close #66821
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
Performance / Flow Control
Bug Fixes
Tests