Repository navigation
br: retain latest mvcc version when merge into one SST - #68020
Conversation
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/invalid-title
release-note-none
|
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:
📝 WalkthroughWalkthroughAdds a --retain-latest-mvcc-version stream restore option: pre-collects compacted SST sets, adds batch-download capability checks and download-path plumbing, updates client init and progress accounting, validates absence of user DML logs for the flag, adds tests, and bumps kvproto pins. ChangesStream restore retain-latest-mvcc-version
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Stream as Stream Restore
participant LogClient as LogClient
participant SnapClient as SnapClient
participant Importer as ImporterClient
participant TiKV as TiKV Stores
User->>Stream: restore --retain-latest-mvcc-version
Stream->>Stream: Probe DML/log iterator (hasAnyLogFiles)
alt DML logs exist
Stream->>User: Error (cannot use flag with user DML logs)
else No DML logs
Stream->>LogClient: InitClients(retainLatestMVCCVersion=true)
LogClient->>SnapClient: NewSnapFileImporterOptions(..., retainLatestMVCCVersion=true)
SnapClient->>Importer: CheckBatchDownloadLatestMVCCSupport(stores)
Importer->>TiKV: BatchDownloadLatestMVCC probe
alt TiKV supports BatchDownloadLatestMVCC
SnapClient->>Importer: BatchDownloadLatestMVCC (download requests)
Importer->>TiKV: Download latest MVCC versions
TiKV->>Importer: DownloadResponse (SSTs)
else TiKV returns Unimplemented
Importer->>SnapClient: Error (Unimplemented → upgrade or disable flag)
SnapClient->>Stream: Error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![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/snap_client/import.go (1)
731-852: Optional: consider extracting shared scaffolding withbatchDownloadNewestVersionSST.The per-peer loop, token acquisition, retry wrapping, empty/error response handling, and the
mu-protected result recording are nearly identical betweenbatchDownloadSSTand the newbatchDownloadNewestVersionSST. The meaningful differences are (1) request grouping shape (map-by-CF vs. one-per-group), (2) which RPC to call, and (3) how to map the response into result metas. Those three points are amenable to a small strategy-style helper, which would reduce drift risk the next time either path evolves.Not required for this PR — leaving as a follow-up suggestion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/pkg/restore/snap_client/import.go` around lines 731 - 852, The per-peer download scaffolding in batchDownloadSST is duplicated in batchDownloadNewestVersionSST; extract a helper (e.g., runPerPeerDownload) that accepts the regionInfo, downloadReqs, importer.downloadTokensMap, importer.concurrencyPerStore, a function parameter to perform the RPC (single vs batch), and a small mapper that converts an RPC response into the SST meta entries to store into resultMetasMap under mu; move token acquire/release, utils.WithRetryV2 wrapper, empty/error response handling, and mu-protected result recording into that helper and call it from both batchDownloadSST and batchDownloadNewestVersionSST, preserving existing request-shape differences by using the mapper and RPC function parameters.
🤖 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/pkg/restore/snap_client/import.go`:
- Around line 731-852: The per-peer download scaffolding in batchDownloadSST is
duplicated in batchDownloadNewestVersionSST; extract a helper (e.g.,
runPerPeerDownload) that accepts the regionInfo, downloadReqs,
importer.downloadTokensMap, importer.concurrencyPerStore, a function parameter
to perform the RPC (single vs batch), and a small mapper that converts an RPC
response into the SST meta entries to store into resultMetasMap under mu; move
token acquire/release, utils.WithRetryV2 wrapper, empty/error response handling,
and mu-protected result recording into that helper and call it from both
batchDownloadSST and batchDownloadNewestVersionSST, preserving existing
request-shape differences by using the mapper and RPC function parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 57a7a555-059b-47e3-a82a-95b15bf2c070
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
DEPS.bzlbr/pkg/restore/internal/import_client/import_client.gobr/pkg/restore/log_client/client.gobr/pkg/restore/snap_client/client.gobr/pkg/restore/snap_client/export_test.gobr/pkg/restore/snap_client/import.gobr/pkg/task/restore.gobr/pkg/task/stream.gogo.mod
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68020 +/- ##
================================================
- Coverage 76.3763% 75.3835% -0.9929%
================================================
Files 2039 2021 -18
Lines 562829 566368 +3539
================================================
- Hits 429868 426948 -2920
- Misses 132045 139418 +7373
+ Partials 916 2 -914
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Leavrth
changed the title
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
br/pkg/task/stream.go (1)
1733-1779:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate
--retain-latest-mvcc-versionbefore restoring any SSTs.Line 1759 starts importing SSTs before the
hasAnyLogFilesresult is consumed at Line 1765. If user DML files exist, this path returnsErrInvalidArgumentonly after data has already been restored, which turns a request-validation failure into a partially applied restore.Suggested fix
- var hasDMLFilesResultCh <-chan hasLogFilesResult - if cfg.RetainLatestMVCCVersion { - ch := make(chan hasLogFilesResult, 1) - hasDMLFilesResultCh = ch - go func() { - hasDMLFiles, err := hasAnyLogFiles(ctx, logFilesIter) - ch <- hasLogFilesResult{hasFiles: hasDMLFiles, err: err} - }() - } + if cfg.RetainLatestMVCCVersion { + hasDMLFiles, err := hasAnyLogFiles(ctx, logFilesIter) + if err != nil { + return errors.Trace(err) + } + if hasDMLFiles { + return errors.Annotatef( + berrors.ErrInvalidArgument, + "%s requires no user DML log files during point restore (DDL files are allowed), but found log files in (%d, %d]", + FlagRetainLatestMVCCVersion, + cfg.StartTS, + cfg.RestoreTS, + ) + } + log.Info("enabled restoring compacted SSTs with newest MVCC versions only; skip user DML log restore", + zap.String("flag", FlagRetainLatestMVCCVersion)) + logFilesIter = iter.FromSlice([]*logclient.LogDataFileInfo{}) + } @@ - if hasDMLFilesResultCh != nil { - result := <-hasDMLFilesResultCh - if result.err != nil { - return errors.Trace(result.err) - } - if result.hasFiles { - return errors.Annotatef( - berrors.ErrInvalidArgument, - "%s requires no user DML log files during point restore (DDL files are allowed), but found log files in (%d, %d]", - FlagRetainLatestMVCCVersion, - cfg.StartTS, - cfg.RestoreTS, - ) - } - log.Info("enabled restoring compacted SSTs with newest MVCC versions only; skip user DML log restore", - zap.String("flag", FlagRetainLatestMVCCVersion)) - logFilesIter = iter.FromSlice([]*logclient.LogDataFileInfo{}) - }🤖 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/stream.go` around lines 1733 - 1779, The code calls client.RestoreSSTFileSets before checking the hasAnyLogFiles result, which can apply SSTs before rejecting an invalid --retain-latest-mvcc-version request; fix by consuming hasDMLFilesResultCh and returning the ErrInvalidArgument if DML files exist before invoking RestoreSSTFileSets. Specifically, when hasDMLFilesResultCh != nil, receive from the channel (using hasAnyLogFiles result), handle result.err and if result.hasFiles return the Annotatef error referencing FlagRetainLatestMVCCVersion, cfg.StartTS and cfg.RestoreTS, and only after that proceed to call client.RestoreSSTFileSets (function names: hasAnyLogFiles, RestoreSSTFileSets, variable hasDMLFilesResultCh).br/pkg/restore/log_client/client.go (1)
332-375:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep SST restore bounded instead of buffering the full iterator.
Line 340 drains the entire SST iterator up front, and Line 369 keeps appending every set into
backupFileSetsbefore any import starts.restoreStreamnow uses this on the normal PITR path, so large restores pay O(total SST metadata) memory and delayed time-to-first-import even when--retain-latest-mvcc-versionis off. Please batch this collection or keep the streaming path for the default flow.🤖 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 332 - 375, CollectSSTFileSets currently drains the entire compactionsIter into memory (backupFileSets) before any import, causing O(total SST metadata) memory and delayed first-import; change it to a bounded/streaming approach: modify CollectSSTFileSets to yield/emit batches instead of returning a full slice — e.g., accept a callback or channel parameter (or an iterator-style return) and emit restore.BackupFileSet batches as they are built (using the existing iter.TryNextor loop), calling rc.rewriteRulesFor and computing totalKVs per batch, or stop/flush a batch once it reaches a configurable size; update callers (e.g., restoreStream) to consume batches and start imports immediately so memory stays bounded and time-to-first-import is reduced while keeping logic in CollectSSTFileSets, iter.TryNextor, rewriteRulesFor and the creation of restore.BackupFileSet intact.
🤖 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.
Outside diff comments:
In `@br/pkg/restore/log_client/client.go`:
- Around line 332-375: CollectSSTFileSets currently drains the entire
compactionsIter into memory (backupFileSets) before any import, causing O(total
SST metadata) memory and delayed first-import; change it to a bounded/streaming
approach: modify CollectSSTFileSets to yield/emit batches instead of returning a
full slice — e.g., accept a callback or channel parameter (or an iterator-style
return) and emit restore.BackupFileSet batches as they are built (using the
existing iter.TryNextor loop), calling rc.rewriteRulesFor and computing totalKVs
per batch, or stop/flush a batch once it reaches a configurable size; update
callers (e.g., restoreStream) to consume batches and start imports immediately
so memory stays bounded and time-to-first-import is reduced while keeping logic
in CollectSSTFileSets, iter.TryNextor, rewriteRulesFor and the creation of
restore.BackupFileSet intact.
In `@br/pkg/task/stream.go`:
- Around line 1733-1779: The code calls client.RestoreSSTFileSets before
checking the hasAnyLogFiles result, which can apply SSTs before rejecting an
invalid --retain-latest-mvcc-version request; fix by consuming
hasDMLFilesResultCh and returning the ErrInvalidArgument if DML files exist
before invoking RestoreSSTFileSets. Specifically, when hasDMLFilesResultCh !=
nil, receive from the channel (using hasAnyLogFiles result), handle result.err
and if result.hasFiles return the Annotatef error referencing
FlagRetainLatestMVCCVersion, cfg.StartTS and cfg.RestoreTS, and only after that
proceed to call client.RestoreSSTFileSets (function names: hasAnyLogFiles,
RestoreSSTFileSets, variable hasDMLFilesResultCh).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9dbac409-2982-47ff-8d97-d7f0bf90a778
📒 Files selected for processing (3)
br/pkg/restore/log_client/client.gobr/pkg/restore/log_client/client_test.gobr/pkg/task/stream.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
br/pkg/task/stream.go (1)
1769-1787:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail-fast validation runs after SST ingestion
Line 1769 restores SST file sets before the retain-latest DML validation at Line 1774, so a disallowed input can partially mutate the target cluster and only then fail with
ErrInvalidArgument.🔧 Suggested fix (validate before any restore-side effects)
- err = client.RestoreSSTFileSets(ctx, sstFileSets, importModeSwitcher, p.IncBy) - if err != nil { - return errors.Trace(err) - } - if hasDMLFilesResultCh != nil { result := <-hasDMLFilesResultCh if result.err != nil { return errors.Trace(result.err) } if result.hasFiles { return errors.Annotatef( berrors.ErrInvalidArgument, "%s requires no user DML log files during point restore (DDL files are allowed), but found log files in (%d, %d]", FlagRetainLatestMVCCVersion, cfg.StartTS, cfg.RestoreTS, ) } log.Info("enabled restoring compacted SSTs with newest MVCC versions only; skip user DML log restore", zap.String("flag", FlagRetainLatestMVCCVersion)) logFilesIter = iter.FromSlice([]*logclient.LogDataFileInfo{}) } + + err = client.RestoreSSTFileSets(ctx, sstFileSets, importModeSwitcher, p.IncBy) + if err != nil { + return errors.Trace(err) + }🤖 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/stream.go` around lines 1769 - 1787, The retain-latest DML validation must run before performing any side-effects; move the hasDMLFilesResultCh check so it executes and returns ErrInvalidArgument (with FlagRetainLatestMVCCVersion and cfg.StartTS/cfg.RestoreTS in the message) prior to calling client.RestoreSSTFileSets. Concretely, evaluate <-hasDMLFilesResultCh, handle result.err and result.hasFiles (returning the annotated berrors.ErrInvalidArgument) before invoking client.RestoreSSTFileSets(ctx, sstFileSets, importModeSwitcher, p.IncBy) to ensure fail-fast validation.
🤖 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.
Outside diff comments:
In `@br/pkg/task/stream.go`:
- Around line 1769-1787: The retain-latest DML validation must run before
performing any side-effects; move the hasDMLFilesResultCh check so it executes
and returns ErrInvalidArgument (with FlagRetainLatestMVCCVersion and
cfg.StartTS/cfg.RestoreTS in the message) prior to calling
client.RestoreSSTFileSets. Concretely, evaluate <-hasDMLFilesResultCh, handle
result.err and result.hasFiles (returning the annotated
berrors.ErrInvalidArgument) before invoking client.RestoreSSTFileSets(ctx,
sstFileSets, importModeSwitcher, p.IncBy) to ensure fail-fast validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d79b6626-2fc6-4284-9874-64502188fec4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
DEPS.bzlbr/pkg/restore/snap_client/client.gobr/pkg/task/restore.gobr/pkg/task/stream.gogo.mod
💤 Files with no reviewable changes (1)
- DEPS.bzl
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68020 +/- ##
================================================
- Coverage 76.3192% 76.0285% -0.2907%
================================================
Files 2041 2052 +11
Lines 561552 571942 +10390
================================================
+ Hits 428572 434839 +6267
- Misses 132076 135830 +3754
- Partials 904 1273 +369
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>
ti-chi-bot
Bot
added
size/XXL
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
approved
needs-1-more-lgtm
|
[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
|
/ok-to-test |
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>
|
/retest |
1 similar comment
|
/retest |
|
/retest |
3 participants
What problem does this PR solve?
Issue Number: ref #64308 close #68905
Problem Summary:
need to retain latest mvcc version when merge into SST to avoid space full.
What changed and how does it work?
retain latest mvcc version when merge into SST
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
Refactor
Bug Fixes
Tests
Chores