br: Adjust restore concurrency and tikv config#69590
Conversation
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/XXL
📝 WalkthroughWalkthroughThis PR tunes BR/PITR restore concurrency defaults, extends log-restore checkpoint metadata with RocksDB and snapshot-size fields, adds a TiKV compacted-SST flow-control adjustment mechanism, temporarily lowers rocksdb.max-background-jobs during restore, and hardens streamhelper's etcd watch/checkpoint operations with retries and synchronized watcher resets. ChangesRestore Concurrency, Checkpoint Metadata, and TiKV Flow Control
Estimated code review effort: 4 (Complex) | ~75 minutes Streamhelper etcd Watch and Checkpoint Robustness
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
|
@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. |
![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 (4)
br/pkg/restore/log_client/client.go (1)
598-601: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
7186sizing rationale.The comment explains the goal, but not why
7186is the safe per-store pool size. Please add the derivation, benchmark basis, or issue reference so future tuning doesn’t regress restore behavior. As per coding guidelines, comments should explain non-obvious constraints and performance trade-offs.🤖 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 598 - 601, The `sstRestoreWorkerPoolSizePerStore` constant in `client.go` uses the magic value `7186` without explaining why it is safe, so add a comment that documents the sizing rationale. Update the nearby restore pool sizing logic to include the derivation, benchmark basis, or issue reference that justifies `7186`, so future changes to the pool size can be tuned without regressing restore behavior.Source: Coding guidelines
br/pkg/restore/log_client/flow_control.go (1)
234-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the config lookup failure.
Line 241 returns the raw SQL error, so restore failures won’t show which TiKV config lookup failed. Add the queried config name for actionable diagnostics. As per coding guidelines, “Go code: Keep error handling actionable and contextual; avoid silently swallowing errors.”
Suggested diff
if errSQL != nil { - return nil, errSQL + return nil, errors.Annotatef(errSQL, "failed to query TiKV config %q", name) }🤖 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/flow_control.go` around lines 234 - 241, The TiKV config lookup in this ExecRestrictedSQL path returns the raw SQL error without context, so restore failures can’t tell which config name failed. Update the error handling in this flow_control.go lookup to wrap or annotate errSQL with the queried name variable before returning it, so the failure from this config query is actionable. Use the existing ExecRestrictedSQL call and the surrounding restore/log client flow as the place to add the contextual message.Source: Coding guidelines
br/pkg/task/restore.go (1)
328-329: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing tags for consistency with sibling unexported fields.
Other unexported fields in this struct (e.g.
tiflashRecorder,tableMappingManager) carry explicitjson:"-" toml:"-"tags even though they're unexported. Consider adding the same tags tosnapshotRestoreDataSizefor consistency with the surrounding style.🤖 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/task/restore.go` around lines 328 - 329, The unexported field snapshotRestoreDataSize should follow the same struct-tag convention as nearby fields like tiflashRecorder and tableMappingManager. Update the restore task struct to add explicit json:"-" toml:"-" tags to snapshotRestoreDataSize so it is treated consistently with the other internal-only fields.br/pkg/checkpoint/checkpoint_test.go (1)
109-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing round-trip test coverage for
SnapshotRestoreDataSize.This test was updated to cover the new
RocksDBMaxBackgroundJobsfield (set + assert save/load + assert viaGetCheckpointTaskInfo), but the sibling new fieldSnapshotRestoreDataSizeadded inbr/pkg/checkpoint/log_restore.goisn't set or asserted here.♻️ Proposed test additions
checkpointMetaForLogRestore := &checkpoint.CheckpointMetadataForLogRestore{ UpstreamClusterID: 123, RestoredTS: 222, StartTS: 111, RewriteTS: 333, GcRatio: "1.0", RocksDBMaxBackgroundJobs: "8", + SnapshotRestoreDataSize: 456, TiFlashItems: map[int64]model.TiFlashReplicaInfo{1: {Count: 1}}, }require.Equal(t, checkpointMetaForLogRestore.RocksDBMaxBackgroundJobs, checkpointMetaForLogRestore2.RocksDBMaxBackgroundJobs) + require.Equal(t, checkpointMetaForLogRestore.SnapshotRestoreDataSize, checkpointMetaForLogRestore2.SnapshotRestoreDataSize) require.Equal(t, checkpointMetaForLogRestore.TiFlashItems, checkpointMetaForLogRestore2.TiFlashItems)Also applies to: 142-150
🤖 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/checkpoint/checkpoint_test.go` around lines 109 - 129, Add round-trip coverage for the new SnapshotRestoreDataSize field in the checkpoint metadata test: in the checkpointMetaForLogRestore setup and the save/load assertions around LogMetaManager.SaveCheckpointMetadata, set SnapshotRestoreDataSize on CheckpointMetadataForLogRestore and verify it is preserved after LoadCheckpointMetadata. Also update the related GetCheckpointTaskInfo assertions in the same test block so the new field is validated alongside RocksDBMaxBackgroundJobs and the other restored metadata fields.
🤖 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/log_client/client.go`:
- Around line 756-762: The return path in the checkpoint metadata reuse logic is
unconditionally using meta.SnapshotRestoreDataSize, which causes older
checkpoints to resume with a zero snapshot size. Update the restore config path
in client.go’s metadata handling to preserve the caller-computed snapshot size
when the metadata field is unset or zero, mirroring the fallback behavior
already used for RocksDBMaxBackgroundJobs, and keep the logic centered around
the existing reuse TiKV config block.
In `@br/pkg/streamhelper/advancer_cliext.go`:
- Around line 570-589: The checkpoint upload path is not idempotent because
advancer_upload_global_checkpoint retries an unconditional KV.Put in
runMetadataRequestWithRetry, which can overwrite a newer checkpoint with an
older one after a commit timeout. Update the upload flow in advancer_cliext.go
to use a compare-and-swap/etcd transaction or a re-read-and-retry-on-conflict
approach inside the request callback so the stored checkpoint only moves forward
and never regresses. Keep the monotonic guard in sync with the retry logic
around t.KV.Put, checkpoint, and redactedKey.
---
Nitpick comments:
In `@br/pkg/checkpoint/checkpoint_test.go`:
- Around line 109-129: Add round-trip coverage for the new
SnapshotRestoreDataSize field in the checkpoint metadata test: in the
checkpointMetaForLogRestore setup and the save/load assertions around
LogMetaManager.SaveCheckpointMetadata, set SnapshotRestoreDataSize on
CheckpointMetadataForLogRestore and verify it is preserved after
LoadCheckpointMetadata. Also update the related GetCheckpointTaskInfo assertions
in the same test block so the new field is validated alongside
RocksDBMaxBackgroundJobs and the other restored metadata fields.
In `@br/pkg/restore/log_client/client.go`:
- Around line 598-601: The `sstRestoreWorkerPoolSizePerStore` constant in
`client.go` uses the magic value `7186` without explaining why it is safe, so
add a comment that documents the sizing rationale. Update the nearby restore
pool sizing logic to include the derivation, benchmark basis, or issue reference
that justifies `7186`, so future changes to the pool size can be tuned without
regressing restore behavior.
In `@br/pkg/restore/log_client/flow_control.go`:
- Around line 234-241: The TiKV config lookup in this ExecRestrictedSQL path
returns the raw SQL error without context, so restore failures can’t tell which
config name failed. Update the error handling in this flow_control.go lookup to
wrap or annotate errSQL with the queried name variable before returning it, so
the failure from this config query is actionable. Use the existing
ExecRestrictedSQL call and the surrounding restore/log client flow as the place
to add the contextual message.
In `@br/pkg/task/restore.go`:
- Around line 328-329: The unexported field snapshotRestoreDataSize should
follow the same struct-tag convention as nearby fields like tiflashRecorder and
tableMappingManager. Update the restore task struct to add explicit json:"-"
toml:"-" tags to snapshotRestoreDataSize so it is treated consistently with the
other internal-only fields.
🪄 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: 6ddf7b31-f24b-4f91-88c4-3add797582bb
📒 Files selected for processing (20)
br/pkg/checkpoint/checkpoint_test.gobr/pkg/checkpoint/log_restore.gobr/pkg/conn/conn.gobr/pkg/conn/conn_test.gobr/pkg/restore/log_client/BUILD.bazelbr/pkg/restore/log_client/client.gobr/pkg/restore/log_client/client_test.gobr/pkg/restore/log_client/export_test.gobr/pkg/restore/log_client/flow_control.gobr/pkg/restore/snap_client/client.gobr/pkg/streamhelper/advancer_cliext.gobr/pkg/streamhelper/client.gobr/pkg/streamhelper/export_test.gobr/pkg/streamhelper/integration_test.gobr/pkg/task/restore.gobr/pkg/task/stream.gobr/pkg/task/stream_test.gobr/pkg/utils/BUILD.bazelbr/pkg/utils/db.gobr/pkg/utils/db_test.go
| if meta.RocksDBMaxBackgroundJobs != "" { | ||
| rocksDBMaxBackgroundJobs = meta.RocksDBMaxBackgroundJobs | ||
| } | ||
| log.Info("reuse TiKV config from checkpoint metadata", | ||
| zap.String("gc-ratio", meta.GcRatio), | ||
| zap.String("rocksdb-max-background-jobs", rocksDBMaxBackgroundJobs)) | ||
| return meta.GcRatio, rocksDBMaxBackgroundJobs, meta.SnapshotRestoreDataSize, nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the computed snapshot size for older checkpoints.
Line 762 returns meta.SnapshotRestoreDataSize unconditionally. Checkpoints created before this field existed deserialize it as 0, unlike RocksDBMaxBackgroundJobs where the caller value is kept when metadata is missing; that feeds a zero snapshot size into the compacted-SST flow-control estimate on resume.
Suggested diff
if meta.RocksDBMaxBackgroundJobs != "" {
rocksDBMaxBackgroundJobs = meta.RocksDBMaxBackgroundJobs
}
+ if meta.SnapshotRestoreDataSize != 0 {
+ snapshotRestoreDataSize = meta.SnapshotRestoreDataSize
+ }
log.Info("reuse TiKV config from checkpoint metadata",
zap.String("gc-ratio", meta.GcRatio),
zap.String("rocksdb-max-background-jobs", rocksDBMaxBackgroundJobs))
- return meta.GcRatio, rocksDBMaxBackgroundJobs, meta.SnapshotRestoreDataSize, nil
+ return meta.GcRatio, rocksDBMaxBackgroundJobs, snapshotRestoreDataSize, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if meta.RocksDBMaxBackgroundJobs != "" { | |
| rocksDBMaxBackgroundJobs = meta.RocksDBMaxBackgroundJobs | |
| } | |
| log.Info("reuse TiKV config from checkpoint metadata", | |
| zap.String("gc-ratio", meta.GcRatio), | |
| zap.String("rocksdb-max-background-jobs", rocksDBMaxBackgroundJobs)) | |
| return meta.GcRatio, rocksDBMaxBackgroundJobs, meta.SnapshotRestoreDataSize, nil | |
| if meta.RocksDBMaxBackgroundJobs != "" { | |
| rocksDBMaxBackgroundJobs = meta.RocksDBMaxBackgroundJobs | |
| } | |
| if meta.SnapshotRestoreDataSize != 0 { | |
| snapshotRestoreDataSize = meta.SnapshotRestoreDataSize | |
| } | |
| log.Info("reuse TiKV config from checkpoint metadata", | |
| zap.String("gc-ratio", meta.GcRatio), | |
| zap.String("rocksdb-max-background-jobs", rocksDBMaxBackgroundJobs)) | |
| return meta.GcRatio, rocksDBMaxBackgroundJobs, snapshotRestoreDataSize, nil |
🤖 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 756 - 762, The return path
in the checkpoint metadata reuse logic is unconditionally using
meta.SnapshotRestoreDataSize, which causes older checkpoints to resume with a
zero snapshot size. Update the restore config path in client.go’s metadata
handling to preserve the caller-computed snapshot size when the metadata field
is unset or zero, mirroring the fallback behavior already used for
RocksDBMaxBackgroundJobs, and keep the logic centered around the existing reuse
TiKV config block.
| _, err = runMetadataRequestWithRetry(ctx, | ||
| "failed to upload global checkpoint to metadata store", | ||
| []zap.Field{ | ||
| zap.String("key", redactedKey), | ||
| zap.String("task", taskName), | ||
| zap.Uint64("checkpoint", checkpoint), | ||
| }, | ||
| func(requestCtx context.Context) (struct{}, error) { | ||
| failpoint.Inject("advancer_upload_global_checkpoint_request_timeout", func() { | ||
| failpoint.Return(struct{}{}, context.DeadlineExceeded) | ||
| }) | ||
| _, err = t.KV.Put(requestCtx, key, value) | ||
| if err == nil { | ||
| failpoint.Inject("advancer_upload_global_checkpoint_commit_timeout", func() { | ||
| err = context.DeadlineExceeded | ||
| }) | ||
| } | ||
| return struct{}{}, err | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the checkpoint upload retry idempotent.
Line 581 retries an unconditional KV.Put after the commit-timeout path at Lines 583-585, where the first Put may already have succeeded. If another advancer writes a higher checkpoint before a retry, this retry can overwrite it with the older checkpoint value despite the monotonic guard at Lines 559-567. Use an etcd transaction/CAS or re-read-on-conflict flow so retries never regress the stored checkpoint.
🤖 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/streamhelper/advancer_cliext.go` around lines 570 - 589, The
checkpoint upload path is not idempotent because
advancer_upload_global_checkpoint retries an unconditional KV.Put in
runMetadataRequestWithRetry, which can overwrite a newer checkpoint with an
older one after a commit timeout. Update the upload flow in advancer_cliext.go
to use a compare-and-swap/etcd transaction or a re-read-and-retry-on-conflict
approach inside the request callback so the stored checkpoint only moves forward
and never regresses. Keep the monotonic guard in sync with the retry logic
around t.KV.Put, checkpoint, and redactedKey.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69590 +/- ##
================================================
- Coverage 76.3268% 75.6362% -0.6907%
================================================
Files 2041 2066 +25
Lines 561003 575568 +14565
================================================
+ Hits 428196 435338 +7142
- Misses 131906 138797 +6891
- Partials 901 1433 +532
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
What problem does this PR solve?
Issue Number: close #69589
Problem Summary:
storage.flow-control.soft-pending-compaction-bytes-limitwill make healthy tikv busy androcksdb.max-background-jobswill consume disk resources when log restore.What changed and how does it work?
--tikv-max-restore-concurrency.storage.flow-control.soft/hard-pending-compaction-bytes-limitandrocksdb.max-background-jobsbefore 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