Repository navigation
br: set ratelimit for log restore (#64357) - #69153
ti-chi-bot[bot] merged 6 commits into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@Leavrth This PR has conflicts, I have hold it. |
|
@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. |
ti-chi-bot
Bot
added
size/L
|
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 Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe PR updates online/offline restore mode handling and adds task-scoped SST download rate limiting. Snapshot and log restore clients now share callback-based rate-limit setup, periodic refresh, reset retries, and updated SST initialization. ChangesRestore mode and online/offline flow
Importer metadata and rate-limit lifecycle
LogClient SST integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RestoreTask
participant LogClient
participant SnapClient
participant SnapFileImporter
participant PD
participant TiKV
RestoreTask->>LogClient: pass online mode and rate limit
LogClient->>SnapClient: register rate-limit callbacks
SnapClient->>SnapFileImporter: open importer
SnapClient->>PD: fetch TiKV stores
PD-->>SnapClient: return stores
SnapClient->>TiKV: apply task-scoped speed limit
SnapClient->>SnapFileImporter: close importer
SnapClient->>TiKV: reset speed limit with retries
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
|
@ti-chi-bot: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
br/pkg/restore/snap_client/client.go (1)
799-827:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve the cherry-pick conflicts across the restore rate-limit path.
These hunks in
br/pkg/restore/snap_client/client.goandbr/pkg/restore/log_client/client.gostill contain unresolved conflict markers, so the branch will not compile. When you keep the newerInitClientspath inbr/pkg/restore/log_client/client.go, also renamesnapFileImporterback to the in-scopefileImporter.🤖 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/snap_client/client.go` around lines 799 - 827, Remove the unresolved git conflict markers and pick the newer rate-limit implementation: replace the old SetSpeedLimitFn block with the SetSpeedLimitCallbacks call (use SetSpeedLimitCallbacks(ctx, rc.pdClient, rc.workerPool, rc.rateLimit) to produce createCallBack and closeCallBack and append them to createCallBacks/closeCallBacks), remove the retry/reset loop variant, and ensure you rename any local variable snapFileImporter back to the in-scope fileImporter to match the InitClients path in log_client (also remove the conflict markers <<<<<<</=======/>>>>>>>).
🧹 Nitpick comments (3)
br/pkg/restore/import_mode_switcher_test.go (1)
51-138: ⚡ Quick winKeep a regression test for the online no-op path.
RestorePreWorkstill has a liveisOnlineearly-return branch, andbr/pkg/task/restore.goplusbr/pkg/task/restore_raw.gostill rely on it. WithRestorePostWorknow always running, removing the online-path test leaves the “no import-mode switch / no scheduler restore needed” contract unverified.As per coding guidelines, "Test files: Prefer extending existing test suites and fixtures over creating new scaffolding; keep test changes minimal and deterministic."
🤖 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/import_mode_switcher_test.go` around lines 51 - 138, Add a deterministic regression test that exercises the early-return "online" path of RestorePreWork and verifies RestorePostWork is a no-op: call restore.RestorePreWork with the isOnline=true flag (using the existing test fixture setup: mgr, switcher, pdHTTPCli), assert that it returns a nil/empty cfg/undo (or otherwise indicates no scheduler changes), confirm pdHTTPCli and pdutil scheduler state is unchanged, then call restore.RestorePostWork(switcher, undo) and re-check pdHTTPCli/pdutil remain unchanged; place this in import_mode_switcher_test.go alongside TestRestorePreWork to reuse existing setup and keep the test minimal and deterministic.Source: Coding guidelines
br/pkg/restore/snap_client/import.go (1)
80-80: ⚡ Quick winAdd doc comments for the newly exported rate-limit APIs.
DownloadRateLimitTTLSeconds,SetSpeedLimitCallbacks,SetSpeedLimitFn, andSetRateLimitare all newly exported; please add package-style doc comments that explain their constraints and intent.As per coding guidelines, "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs" and "Keep exported-symbol doc comments, and prefer semantic constraints over name restatement."
🤖 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/snap_client/import.go` at line 80, Add package-style doc comments for the newly exported symbols DownloadRateLimitTTLSeconds, SetSpeedLimitCallbacks, SetSpeedLimitFn, and SetRateLimit: describe the units and purpose of DownloadRateLimitTTLSeconds (seconds, TTL for cached rate limits), specify expected behavior and thread-safety/locking assumptions for SetSpeedLimitCallbacks and SetSpeedLimitFn (what callbacks/signatures are invoked, whether they may be called concurrently, and if they must be non-blocking), and document SetRateLimit’s contract (valid range/units for rate values, effect on existing transfers, and how TTL interacts with explicit sets). Keep comments short, state constraints/invariants, and use package-style sentences above each exported declaration.Source: Coding guidelines
br/pkg/restore/snap_client/client_test.go (1)
428-471: 💤 Low valueGood idempotency test, consider adding a doc comment.
The test correctly verifies that
closeCallBackcan be called multiple times without blocking or erroring, even after context cancellation. Consider adding a brief comment at the start of the test explaining this intent for future readers.📝 Suggested doc comment
func TestSetSpeedLimitCloseCallbackIdempotent(t *testing.T) { + // Verify that the close callback returned by SetSpeedLimitCallbacks is idempotent + // and can be safely called multiple times without blocking or error. mockStores := []*metapb.Store{🤖 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/snap_client/client_test.go` around lines 428 - 471, Add a brief doc comment at the top of TestSetSpeedLimitCloseCallbackIdempotent that explains the test verifies idempotency and non-blocking behavior of the closeCallBack (after context cancellation) and that calling closeCallBack multiple times should return quickly without error; place the comment immediately above the TestSetSpeedLimitCloseCallbackIdempotent function and mention the relevant symbols createCallBack and closeCallBack so readers know the test's intent.
🤖 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/pkg/restore/snap_client/client.go`:
- Around line 693-745: The refresh goroutine uses the long-lived ctx for setFn
calls and can block shutdown; modify the code so the ticker loop creates a
bounded/cancelable child context for each setFn call (e.g. ctxRefresh, cancel :=
context.WithTimeout(ctx, refreshCallTimeout)) and pass ctxRefresh to
setFn(importer, rateLimit), deferring cancel immediately after the call, and
also have an outer cancel (ctxLoopCancel) created with context.WithCancel(ctx)
that's stored outside the goroutine; in the close callback (the second returned
func) call ctxLoopCancel() (or cancel) before closing stopCh and wg.Wait() to
ensure any in-flight setFn calls are canceled and the goroutine can exit
promptly; reference SetSpeedLimitFn, setFn, SnapFileImporter, stopCh, stopOnce,
wg, updateTicker and ctx when locating changes.
---
Outside diff comments:
In `@br/pkg/restore/snap_client/client.go`:
- Around line 799-827: Remove the unresolved git conflict markers and pick the
newer rate-limit implementation: replace the old SetSpeedLimitFn block with the
SetSpeedLimitCallbacks call (use SetSpeedLimitCallbacks(ctx, rc.pdClient,
rc.workerPool, rc.rateLimit) to produce createCallBack and closeCallBack and
append them to createCallBacks/closeCallBacks), remove the retry/reset loop
variant, and ensure you rename any local variable snapFileImporter back to the
in-scope fileImporter to match the InitClients path in log_client (also remove
the conflict markers <<<<<<</=======/>>>>>>>).
---
Nitpick comments:
In `@br/pkg/restore/import_mode_switcher_test.go`:
- Around line 51-138: Add a deterministic regression test that exercises the
early-return "online" path of RestorePreWork and verifies RestorePostWork is a
no-op: call restore.RestorePreWork with the isOnline=true flag (using the
existing test fixture setup: mgr, switcher, pdHTTPCli), assert that it returns a
nil/empty cfg/undo (or otherwise indicates no scheduler changes), confirm
pdHTTPCli and pdutil scheduler state is unchanged, then call
restore.RestorePostWork(switcher, undo) and re-check pdHTTPCli/pdutil remain
unchanged; place this in import_mode_switcher_test.go alongside
TestRestorePreWork to reuse existing setup and keep the test minimal and
deterministic.
In `@br/pkg/restore/snap_client/client_test.go`:
- Around line 428-471: Add a brief doc comment at the top of
TestSetSpeedLimitCloseCallbackIdempotent that explains the test verifies
idempotency and non-blocking behavior of the closeCallBack (after context
cancellation) and that calling closeCallBack multiple times should return
quickly without error; place the comment immediately above the
TestSetSpeedLimitCloseCallbackIdempotent function and mention the relevant
symbols createCallBack and closeCallBack so readers know the test's intent.
In `@br/pkg/restore/snap_client/import.go`:
- Line 80: Add package-style doc comments for the newly exported symbols
DownloadRateLimitTTLSeconds, SetSpeedLimitCallbacks, SetSpeedLimitFn, and
SetRateLimit: describe the units and purpose of DownloadRateLimitTTLSeconds
(seconds, TTL for cached rate limits), specify expected behavior and
thread-safety/locking assumptions for SetSpeedLimitCallbacks and SetSpeedLimitFn
(what callbacks/signatures are invoked, whether they may be called concurrently,
and if they must be non-blocking), and document SetRateLimit’s contract (valid
range/units for rate values, effect on existing transfers, and how TTL interacts
with explicit sets). Keep comments short, state constraints/invariants, and use
package-style sentences above each exported declaration.
🪄 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: 38c3818e-58f7-4bbc-b675-1185750a10ab
📒 Files selected for processing (14)
br/pkg/restore/BUILD.bazelbr/pkg/restore/import_mode_switcher.gobr/pkg/restore/import_mode_switcher_test.gobr/pkg/restore/log_client/client.gobr/pkg/restore/snap_client/BUILD.bazelbr/pkg/restore/snap_client/client.gobr/pkg/restore/snap_client/client_test.gobr/pkg/restore/snap_client/export_test.gobr/pkg/restore/snap_client/import.gobr/pkg/restore/snap_client/import_test.gobr/pkg/task/restore.gobr/pkg/task/restore_raw.gobr/pkg/task/restore_txn.gobr/pkg/task/stream.go
| var wg sync.WaitGroup | ||
| stopCh := make(chan struct{}) | ||
| var stopOnce sync.Once | ||
| setFn := SetSpeedLimitFn(ctx, pdClient, pool) | ||
| return func(importer *SnapFileImporter) error { | ||
| if err := setFn(importer, rateLimit); err != nil { | ||
| return errors.Annotate(err, "failed to set download speed limit") | ||
| } | ||
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| updateTicker := time.NewTicker(time.Minute * 3) | ||
| defer updateTicker.Stop() | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-updateTicker.C: | ||
| if err := setFn(importer, rateLimit); err != nil { | ||
| log.Warn("failed to set download speed limit, retry it", zap.Error(err)) | ||
| } | ||
| case <-stopCh: | ||
| return | ||
| } | ||
| } | ||
| }() | ||
| return nil | ||
| }, func(importer *SnapFileImporter) error { | ||
| stopOnce.Do(func() { | ||
| close(stopCh) | ||
| }) | ||
| wg.Wait() | ||
|
|
||
| // In future we may need a mechanism to set speed limit in ttl. like what we do in switchmode. TODO | ||
| var resetErr error | ||
| for retry := range resetSpeedLimitRetryTimes { | ||
| resetCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), resetSpeedLimitTimeout) | ||
| resetErr = SetSpeedLimitFn(resetCtx, pdClient, pool)(importer, 0) | ||
| cancel() | ||
| if resetErr != nil { | ||
| log.Warn("failed to reset speed limit, retry it", | ||
| zap.Int("retry time", retry), logutil.ShortError(resetErr)) | ||
| time.Sleep(time.Duration(retry+3) * time.Second) | ||
| continue | ||
| } | ||
| break | ||
| } | ||
| if resetErr != nil { | ||
| log.Error("failed to reset speed limit, please reset it manually", zap.Error(resetErr)) | ||
| } | ||
| return resetErr | ||
| } | ||
| } |
There was a problem hiding this comment.
Close can hang behind an in-flight refresh.
The ticker goroutine runs setFn with the long-lived restore context, but the close callback only closes stopCh and then waits on wg. If PD lookup or any SetDownloadSpeedLimit RPC stalls while that context is still alive, the goroutine never gets back to the select, and importer shutdown blocks indefinitely. Give the refresh loop its own cancelable/timeout-bounded context and cancel it before wg.Wait().
🤖 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/snap_client/client.go` around lines 693 - 745, The refresh
goroutine uses the long-lived ctx for setFn calls and can block shutdown; modify
the code so the ticker loop creates a bounded/cancelable child context for each
setFn call (e.g. ctxRefresh, cancel := context.WithTimeout(ctx,
refreshCallTimeout)) and pass ctxRefresh to setFn(importer, rateLimit),
deferring cancel immediately after the call, and also have an outer cancel
(ctxLoopCancel) created with context.WithCancel(ctx) that's stored outside the
goroutine; in the close callback (the second returned func) call ctxLoopCancel()
(or cancel) before closing stopCh and wg.Wait() to ensure any in-flight setFn
calls are canceled and the goroutine can exit promptly; reference
SetSpeedLimitFn, setFn, SnapFileImporter, stopCh, stopOnce, wg, updateTicker and
ctx when locating changes.
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
🧹 Nitpick comments (1)
br/pkg/restore/log_client/client.go (1)
559-560: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a dedicated pool for speed-limit callbacks.
SetSpeedLimitCallbacksperiodically reapplies and later resets the limit through the pool passed at Line 560, while the samesstWorkerPoolis consumed by SST restore work at Lines 584 and 587. If restore work saturates that pool, the refresh/reset RPCs can queue behind data restore tasks, letting the configured limit expire or delaying cleanup. Consider a small control-plane pool sized by store count.Proposed direction
if rc.rateLimit != 0 { - createCallBack, closeCallBack := snapclient.SetSpeedLimitCallbacks(ctx, rc.pdClient, sstWorkerPool, rc.rateLimit) + speedLimitPoolSize := uint(len(stores)) + if speedLimitPoolSize == 0 { + speedLimitPoolSize = 1 + } + speedLimitWorkerPool := tidbutil.NewWorkerPool(speedLimitPoolSize, "sst speed limit") + createCallBack, closeCallBack := snapclient.SetSpeedLimitCallbacks(ctx, rc.pdClient, speedLimitWorkerPool, rc.rateLimit) createCallBacks = append(createCallBacks, createCallBack) closeCallBacks = append(closeCallBacks, closeCallBack) }Also applies to: 582-587
🤖 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/log_client/client.go` around lines 559 - 560, The speed-limit callback scheduling in rc.rateLimit setup is using the same sstWorkerPool as SST restore work, which can let refresh/reset RPCs get blocked behind data transfer tasks. Update the SetSpeedLimitCallbacks call in client.go to use a dedicated small control-plane pool instead of sstWorkerPool, and wire that pool through the restore path where SST work is started so callback reapply/reset operations stay responsive even under heavy restore load.
🤖 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/log_client/client.go`:
- Around line 559-560: The speed-limit callback scheduling in rc.rateLimit setup
is using the same sstWorkerPool as SST restore work, which can let refresh/reset
RPCs get blocked behind data transfer tasks. Update the SetSpeedLimitCallbacks
call in client.go to use a dedicated small control-plane pool instead of
sstWorkerPool, and wire that pool through the restore path where SST work is
started so callback reapply/reset operations stay responsive even under heavy
restore load.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 60c6a331-2fdc-4fd2-9f4e-15f8932a1369
📒 Files selected for processing (2)
br/pkg/restore/log_client/client.gobr/pkg/restore/snap_client/client.go
💤 Files with no reviewable changes (1)
- br/pkg/restore/snap_client/client.go
ti-chi-bot
Bot
added
cherry-pick-approved
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
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/pkg/restore/import_mode_switcher.go`:
- Around line 247-250: Update the canceled-context handling in RestorePostWork
so post-work cleanup uses a finite timeout context rather than unbounded
context.Background(). Apply that bounded context to both SwitchToNormalMode and
restoreSchedulers, preserving the existing cancellation behavior and ensuring
the timeout context is released after cleanup.
🪄 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 Plus
Run ID: f2acabf3-52c0-470f-a178-371f0217c9ca
📒 Files selected for processing (5)
br/pkg/restore/import_mode_switcher.gobr/pkg/restore/import_mode_switcher_test.gobr/pkg/restore/log_client/client.gobr/pkg/task/restore.gobr/pkg/task/stream.go
🚧 Files skipped from review as they are similar to previous changes (1)
- br/pkg/restore/log_client/client.go
| if ctx.Err() != nil { | ||
| log.Warn("context canceled, try shutdown") | ||
| ctx = context.Background() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect context propagation and retry behavior in cleanup operations.
rg -n -A80 -B10 'func \(switcher \*ImportModeSwitcher\) switchTiKVMode|func \(switcher \*ImportModeSwitcher\) SwitchToNormalMode' \
br/pkg/restore/import_mode_switcher.go
# Locate scheduler undo implementations and inspect their context handling.
rg -n -A40 -B10 'func .*MakeUndoFunctionByConfig|func .*RemoveSchedulersWithConfig|type UndoFunc' br/pkgRepository: pingcap/tidb
Length of output: 14690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact context cleanup path and surrounding deferred undo context.
sed -n '220,265p' br/pkg/restore/import_mode_switcher.go
# Inspect scheduler restore implementation and context handling.
sed -n '620,725p' br/pkg/pdutil/pd.go
# Search for callers of restore deferred cleanup to see whether a timeout is already applied later.
rg -n -A20 -B20 'defer .*undo|defer.*\.Apply|RemoveSchedulers\(ctx\)|RestoreSchedulers' br/pkg/restore br/pkg/lightning br/pkgRepository: pingcap/tidb
Length of output: 20458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the full relevant import mode switcher cleanup path.
sed -n '1,90p' br/pkg/restore/import_mode_switcher.go
sed -n '230,255p' br/pkg/restore/import_mode_switcher.go
# Read scheduler restore implementation and helper request behavior.
sed -n '660,730p' br/pkg/pdutil/pd.go
# Inspect how pdcontroller request methods pass the context.
rg -n -A30 'func \(p \*PdController\) (GetAllStores|GetPDScheduleConfig|ListSchedulers|UpdatePDScheduleConfig|putPDScheduleConfig|doUpdatePDScheduleConfig|getPDScheduleConfig)' br/pkg/pdutil/pd.goRepository: pingcap/tidb
Length of output: 7877
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read scheduler resume and remaining restore path after scheduling.
sed -n '730,790p' br/pkg/pdutil/pd.goRepository: pingcap/tidb
Length of output: 2695
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the implementation that resumes schedulers and how the generated undo calls the restore path.
sed -n '396,435p' br/pkg/pdutil/pd.go
sed -n '330,365p' br/pkg/pdutil/pd.go
sed -n '465,484p' br/pkg/pdutil/pd.goRepository: pingcap/tidb
Length of output: 3773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the scheduler resume implementation and the remaining restoreSchedulers tail.
sed -n '300,370p' br/pkg/pdutil/pd.go
sed -n '734,780p' br/pkg/pdutil/pd.goRepository: pingcap/tidb
Length of output: 4701
Bound the post-work cleanup context.
RestorePostWork replaces a canceled ctx with an unbounded context.Background() before calling SwitchToNormalMode and restoreSchedulers. Both paths can make remote PD/TiKV requests. Apply a finite timeout to these cleanup calls so an unavailable endpoint cannot block restore completion indefinitely.
🤖 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/import_mode_switcher.go` around lines 247 - 250, Update the
canceled-context handling in RestorePostWork so post-work cleanup uses a finite
timeout context rather than unbounded context.Background(). Apply that bounded
context to both SwitchToNormalMode and restoreSchedulers, preserving the
existing cancellation behavior and ensuring the timeout context is released
after cleanup.
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
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 `@DEPS.bzl`:
- Around line 5987-5993: Update the com_github_pingcap_kvproto archive
declaration in DEPS.bzl by adding a reachable public Go module proxy URL to the
fallback list, replacing or removing the 404ing
storage.googleapis.com/pingcapmirror URL as appropriate. Verify that the sha256
value matches the archive served for version v0.0.0-20260803062813-07aa8c6a46fa,
while preserving the existing internal mirrors.
🪄 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 Plus
Run ID: caa8f054-c2d8-4c71-b4d6-66cee954aa66
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
DEPS.bzlgo.mod
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #69153 +/- ##
================================================
Coverage ? 55.7791%
================================================
Files ? 1849
Lines ? 666891
Branches ? 0
================================================
Hits ? 371986
Misses ? 267264
Partials ? 27641
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/test unit-test |
1 similar comment
|
/test unit-test |
|
/test check_dev_2 |
|
@Leavrth: The specified target(s) for The following commands are available to trigger optional jobs: Use 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. |
|
/test check-dev2 |
|
/retest |
|
/test pull-br-integration-test |
4 participants
This is an automated cherry-pick of #64357 and #70221
What problem does this PR solve?
Issue Number: close #63505
Problem Summary: the download speed of log apply is not limited under the ratelimit
What changed and how does it work?
set rate limit for log restore
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
Bug Fixes