store: add migration version import export#1085
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (43b4d73):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for range migrations by implementing ExportVersions, ImportVersions, and MigrationHLCFloor across the storage implementations, allowing raw MVCC versions (including tombstones and TTL metadata) to be exported and imported idempotently. The review feedback highlights a potential synchronization/performance improvement by using atomic operations for updating s.lastCommitTS, and points out an O(N^2) complexity issue in finishExportIfLimited when calculating exported version sizes, suggesting tracking the size incrementally instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (edf74ff):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edf74ffa0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| iterOpts := &pebble.IterOptions{ | ||
| LowerBound: encodeKey(opts.StartKey, math.MaxUint64), | ||
| } | ||
| if opts.EndKey != nil { | ||
| iterOpts.UpperBound = encodeKey(opts.EndKey, math.MaxUint64) |
There was a problem hiding this comment.
For Pebble, bounding the iterator with encodeKey(start/end, MaxUint64) compares against the raw encoded key (userKey || invertedTS), not just the user key, so prefix-related keys cross the range boundary. For example, exporting ["aa", "b") can still emit key "a" because "a" || ^ts sorts after "aa" || 0, while exporting [nil, "aa") can omit the same "a" row; range migration would then copy keys from outside the bracket or miss keys inside it. The export loop needs to either avoid these raw bounds or re-check userKey >= StartKey in addition to the existing end check.
Useful? React with 👍 / 👎.
| migrationAcks map[string]migrationImportAck | ||
| migrationHLCFloors map[uint64]uint64 |
There was a problem hiding this comment.
These new maps live outside the snapshot tree, but restoreStreamingSnapshot only replaces tree, lastCommitTS, and minRetainedTS. If an in-memory store imports a migration batch and then restores a snapshot from before that import, the imported versions are gone while the stale ack/floor remain, so retrying the same batch_seq is reported as a duplicate (or later batches see a false high-water mark) and MigrationHLCFloor can return a floor for data that was rolled back. Restore should clear or rebuild this metadata together with the tree.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (e3079c0):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
@codex review Addressed current-head migration export blockers in store:
Validation:
|
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3079c09fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if opts.EndKey != nil && bytes.Compare(userKey, opts.EndKey) >= 0 { | ||
| _ = s.skipToNextUserKey(iter, userKey) | ||
| return false, true, nil |
There was a problem hiding this comment.
When a bounded export reaches a decoded key at or beyond EndKey, this branch skips to the next user key and keeps scanning without updating ScannedBytes or NextCursor. Because the iterator is created without an upper bound, an empty or small bracket such as [a,b) on a Pebble DB with many later keys can walk the rest of the LSM in a single call, and MaxScannedBytes cannot break it into chunks because these skipped rows are not counted. This can make migration export RPCs time out; either terminate when it is safe or charge/cursor the skipped positions.
Useful? React with 👍 / 👎.
| return true, true, nil | ||
| } | ||
| if opts.StartKey != nil && bytes.Compare(userKey, opts.StartKey) < 0 { | ||
| _ = s.skipToNextUserKey(iter, userKey) |
There was a problem hiding this comment.
When this path skips a decoded key that sorts before StartKey (for example exporting from aa with existing keys a and b), userKey is a slice into Pebble's iterator buffer. skipToNextUserKey repositions the same iterator, so that slice can mutate to the next key and the helper then advances once more, dropping that key's newest version from the export; the same aliasing affects the other skip calls in this function. Clone the decoded user key before passing it to a helper that seeks the iterator.
Useful? React with 👍 / 👎.
| return (len(rawKey) == len(migrationAckPrefix)+migrationAckKeyIDBytes && bytes.HasPrefix(rawKey, []byte(migrationAckPrefix))) || | ||
| (len(rawKey) == len(migrationHLCFloorPrefix)+migrationUint64Bytes && bytes.HasPrefix(rawKey, []byte(migrationHLCFloorPrefix))) |
There was a problem hiding this comment.
This predicate classifies any raw Pebble key with the migration prefix and length as metadata, but a normal MVCC entry is encoded as userKey || invertedTS. A real user key such as !migstage|ack| plus eight bytes therefore has an encoded raw key with exactly this prefix/length and is skipped by export/scan/compaction through isPebbleMetaKey, even though GetAt can still read it. Use an encoding or exact metadata marker that cannot collide with encoded user keys.
Useful? React with 👍 / 👎.
| if err := batch.Set(migrationAckKey(opts.JobID, opts.BracketID), encodeMigrationImportAck(migrationImportAck{ | ||
| batchSeq: opts.BatchSeq, | ||
| cursor: opts.Cursor, | ||
| }), nil); err != nil { |
There was a problem hiding this comment.
Persisting the import ack as an ordinary Pebble key means native Pebble snapshots stream it along with data. If a snapshot is taken after importing job/bracket (J,B) and restored into another store, ImportVersions for batch 1 of (J,B) is reported as a duplicate with the old cursor and the new batch's versions are skipped, even though the restored data image should not carry target-local migration progress. Store these acks outside the snapshot stream or filter them when writing/restoring snapshots.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (643a5a6):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
対応内容:
検証:
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 643a5a6a5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| func normalizeExportVersionsOptions(opts ExportVersionsOptions) ExportVersionsOptions { | ||
| if opts.AcceptKey != nil && opts.MaxScannedBytes == 0 { |
There was a problem hiding this comment.
When a caller exports a bounded snapshot/delta with MaxCommitTSInclusive set but no AcceptKey, versions newer than the ceiling are rejected by the export filters and therefore do not count toward MaxVersions or MaxBytes. Because this normalization leaves MaxScannedBytes at zero in that case, a hot key or prefix with many post-ceiling versions can be scanned to exhaustion in a single ExportVersions call instead of yielding a progress cursor; the defensive scan-budget default should not depend only on AcceptKey.
Useful? React with 👍 / 👎.
| return false, true, err | ||
| } | ||
| if commitTS <= opts.MinCommitTSExclusive { | ||
| _ = s.skipToNextUserKey(iter, userKey) |
There was a problem hiding this comment.
When the latest version for a Pebble user key is at or below MinCommitTSExclusive, this branch uses skipToNextUserKey, which seeks to the synthetic key userKey + 0xff.... For binary keys where a distinct in-range key has the current key as a prefix and sorts before that marker (for example a key like a\xff... after pruning key a), the export jumps over that key entirely, so a delta export can miss versions that should be migrated; advance only over exact matches for the current user key instead of seeking to the prefix upper marker here.
Useful? React with 👍 / 👎.
| if commitTS <= opts.MinCommitTSExclusive { | ||
| _ = s.skipToNextUserKey(iter, userKey) |
There was a problem hiding this comment.
When exporting a delta over a mostly unchanged range, every key whose newest version is already <= MinCommitTSExclusive takes this branch before updating ScannedBytes or NextCursor. Even if the caller set MaxScannedBytes, a chunk can therefore walk the entire range of old keys and return done=true with no progress cursor, defeating the sparse-scan pacing that migration relies on for large unchanged brackets; record the skipped position and honor the scan budget before moving past the key.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (061275a):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
Addressed the current migration export scan blockers in 061275a. Changes:
Caller audit:
Validation:
|
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 061275ab2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return false, false, errors.WithStack(err) | ||
| } | ||
| rawKey := iter.Key() | ||
| if isPebbleMetaKey(rawKey) { |
There was a problem hiding this comment.
When the Pebble DB contains encryption writer-registry rows (!encryption|writers|...), a whole-range export starting at StartKey == nil will iterate those raw non-MVCC keys before normal user keys. Because this guard only skips the existing meta/migration keys, decodeExportedPebbleVersion then parses the 12-byte registry value as an MVCC value, which can either fail the export with a value-header error or emit a bogus version for import. Migration exports of encrypted stores need to exclude this metadata prefix before decoding user versions.
Useful? React with 👍 / 👎.
| if userKey == nil || pebbleExportCursorEqual(pos, userKey, commitTS) { | ||
| return true, true, nil |
There was a problem hiding this comment.
When skipPebbleExportVersionBelowMinTS hits MaxScannedBytes, the cursor it returns names the first version at or below MinCommitTSExclusive, which means the entire rest of that user key is known to be outside the export window. On resume this equality check skips only that exact version and then continues with older versions of the same key, so a hot key with many old versions can produce one empty scan-budget chunk per obsolete version instead of advancing to the next user key. If the cursor timestamp is already <= MinCommitTSExclusive, resume should skip the rest of that key.
Useful? React with 👍 / 👎.
| for iter.First(); iter.Valid(); iter.Next() { | ||
| k := iter.Key() | ||
| v := iter.Value() |
There was a problem hiding this comment.
kvFSM.Snapshot delegates to store.Snapshot, so this filter also applies to Raft snapshots, not just ad-hoc backups. During an in-flight import, dropping _migack / _mighlc from the snapshot makes a restored voter forget the per-bracket high-water mark and HLC floor; after a leader change it can reject the next batch as ErrImportBatchGap or lose the target-local floor needed after cutover. Migration metadata that is part of the target group's applied state needs to survive FSM snapshot/restore, or the backup/export path needs a separate filter.
Useful? React with 👍 / 👎.
| for prefixLen := range userKey { | ||
| prefix := userKey[:prefixLen] | ||
| if (startKey == nil || bytes.Compare(prefix, startKey) >= 0) && bytes.Compare(prefix, endKey) < 0 { |
There was a problem hiding this comment.
For a first-route export with StartKey == nil and a non-empty EndKey, the prefixLen == 0 iteration makes the empty prefix look like a possible in-range key, so this returns false for every key at or beyond EndKey. skipPebbleExportKeyOutsideRange then skips those out-of-range keys one user key at a time without charging ScannedBytes, meaning the final page for [nil, EndKey) can walk the entire rest of the Pebble DB to EOF despite the requested range and any scan budget.
Useful? React with 👍 / 👎.
|
@codex review Addressed the current migration export/snapshot findings in 31ed507:
Caller audit:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (31ed507):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31ed5074c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bytes.Equal(rawKey, metaAppliedIndexBytes) | ||
| bytes.Equal(rawKey, metaAppliedIndexBytes) || | ||
| isMigrationMetadataKey(rawKey) || | ||
| bytes.HasPrefix(rawKey, encryption.WriterRegistryPrefix) |
There was a problem hiding this comment.
When a user key starts with !encryption|writers|, its MVCC Pebble row is encoded as userKey || timestamp, so this new prefix check treats the committed version as writer-registry metadata and skips it during Pebble ExportVersions. PutAt still accepts such keys and the in-memory exporter returns them, so a migration from a Pebble store can silently omit data for those keys; restrict the skip to actual registry-row shapes or reject this namespace before writes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f624305ee0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # kv/shard_store.go # kv/shard_store_test.go
6b40846 to
644fd08
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93dfbb3c44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| s.migrationAcks = make(map[migrationAckID]migrationImportAck) | ||
| s.migrationHLCFloors = make(map[uint64]uint64) |
There was a problem hiding this comment.
On the in-memory MVCC store path, if a snapshot is taken after ImportVersions has acknowledged a migration batch, restoring that snapshot rebuilds the data tree but then drops the snapshotted ack/floor state because writeSnapshotBody never writes these maps. The restored target will accept the same BatchSeq: 1 again as non-duplicate and MigrationHLCFloor returns 0, diverging from the Pebble implementation and breaking import idempotency for tests or any in-memory shard; the snapshot format needs to serialize and restore these maps rather than always recreating them empty.
Useful? React with 👍 / 👎.
| rpc GetTimestamp (GetTimestampRequest) returns (GetTimestampResponse) {} | ||
| rpc ListRoutes (ListRoutesRequest) returns (ListRoutesResponse) {} | ||
| rpc SplitRange (SplitRangeRequest) returns (SplitRangeResponse) {} | ||
| rpc StartSplitMigration (StartSplitMigrationRequest) returns (StartSplitMigrationResponse) {} |
There was a problem hiding this comment.
With this method now in the public Distribution service, generated clients can call StartSplitMigration, but the registered adapter.DistributionServer still only gets this method from its embedded pb.UnimplementedDistributionServer and repo-wide search shows no server implementation for it or the sibling split-job RPCs. In deployments registering NewDistributionServer, starting a migration through the advertised API will return gRPC Unimplemented; either wire the server methods in this change or keep the RPCs out of the service until they are usable.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func isRetryableTransactWriteError(err error) bool { | ||
| return errors.Is(err, store.ErrWriteConflict) || errors.Is(err, kv.ErrTxnLocked) || errors.Is(err, kv.ErrRouteWriteFenced) |
There was a problem hiding this comment.
This retry predicate only recognizes a typed kv.ErrRouteWriteFenced, but follower-routed writes that go through Coordinate.redirect receive the leader's Internal.Forward failure as a gRPC status with the original error chain stripped; the Redis retry code already needs explicit wire parsing for write conflicts/locks for the same path. When a DynamoDB/SQS mutation is served locally on a follower and hits a route fence, it now falls out of the retry loop as a non-retryable server error instead of waiting for migration; add a wire/status matcher for the route-fence error wherever these new retry branches are used.
Useful? React with 👍 / 👎.
…to design/hotspot-split-m2-store-export
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (1df401d):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (4a04a83):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
対応しました。 変更内容:
検証:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (c7dd014):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
追加でCIのJepsen Test起動失敗を修正しました。 変更内容:
追加検証:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (dcb7e2c):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
対応内容:
検証:
GitHub確認:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcb7e2cc9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Evidence for current HEAD db7493c: Addressed the latest findings:
Caller audit:
Validation:
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (db7493c):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Tests
Author: bootjp