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 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. |
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 (84704f9):
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: 84704f947f
ℹ️ 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 len(elems)+1 > maxWideColumnItems { | ||
| return nil, errors.Wrapf(ErrCollectionTooLarge, "delta key count exceeds %d", maxWideColumnItems) | ||
| } | ||
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: pair.Key}) |
There was a problem hiding this comment.
This helper is now used by scanListDeltaDelElems for legacy list deltas. ShardStore annotates each scanned KV with RouteGroupID so deletes can be sent back to the group where that row was read; by dropping pair.RouteGroupID here, a DEL/overwrite that finds a legacy delta on a non-current/source group during migration sends the tombstone to the key's current route instead. The scanned row remains and can still be counted by legacy delta scans, so deleted lists may reappear or keep an incorrect length.
Useful? React with 👍 / 👎.
| if acceptedDeltas > store.MaxDeltaScanLimit { | ||
| return nil, ErrDeltaScanTruncated | ||
| } | ||
| existingDeltas = append(existingDeltas, kv.Key) |
There was a problem hiding this comment.
When loadListState scans legacy delta prefixes, each returned KV can carry the RouteGroupID of the group that actually held the row. Storing only kv.Key means appendListDeletionElems later emits unpinned deletes; in a MULTI/EXEC delete or purge during migration, legacy deltas read from an old/source group can be deleted on the current key route instead, leaving the original delta visible to future list reads.
Useful? React with 👍 / 👎.
| return routeKeyInRange(store.ExtractLegacyListUserKeyFromDelta(rawKey), routeStart, routeEnd) || | ||
| routeKeyInRange(store.ExtractListUserKey(rawKey), routeStart, routeEnd) |
There was a problem hiding this comment.
For an actual legacy delta key, ExtractListUserKey(rawKey) also succeeds because the legacy delta namespace sits under !lst|meta|, but it returns the synthetic d|... base-meta user key rather than the list's logical key. This OR therefore exports the same real delta both for the list owner and for any route covering that synthetic key (for example a split/move of the d| range), which can duplicate the delta onto an unrelated target and make broad legacy scans count it twice.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func pebbleExportCanStopAtEndKey(startKey, endKey, userKey []byte) bool { | ||
| if startKey == nil { |
There was a problem hiding this comment.
Changing this guard to allow an explicitly empty StartKey lets pebbleExportCanStopAtEndKey return true as soon as it sees userKey >= EndKey. Because Pebble encodes rows as userKey || inverted_ts, versions for the empty user key can sort after non-empty keys like b; exporting the leading route ["", "b") can therefore stop at b and miss the empty key even though it belongs to the range.
Useful? React with 👍 / 👎.
| gid, ok := c.router.ResolveGroup(mut.Key) | ||
| if !ok { | ||
| return nil, nil, errors.Wrapf(ErrInvalidRequest, "no route for key %q", mut.Key) | ||
| gid := req.GroupID |
There was a problem hiding this comment.
When a transaction includes an elem with GroupID and that key becomes the lexicographic primary, this groups the mutation under the preserved group, but the 2PC commit path still chooses the primary group with engineGroupIDForKey(primaryKey). If that key's current route has moved away from the preserved group, commitPrimaryTxn sends a COMMIT with only txn metadata and no key mutations, which the FSM rejects after PREPARE; this breaks the migrated legacy-delta compactions/deletes that GroupID was added to support.
Useful? React with 👍 / 👎.
| } | ||
| for i := last - 1; i >= 0; i-- { | ||
| if rawKVs[i] != nil { | ||
| return rawKVs[i].Key |
There was a problem hiding this comment.
When the split guard removes a below-threshold tail key (lastScannedKey == nil) and that tail key has two or more accepted deltas at the end of a truncated page, this helper returns the immediately previous raw row even if it is another accepted delta for the same tail key. The next tick starts after that row, so the tail key is not re-scanned as a complete set with deltas beyond the page boundary, defeating the split guard and leaving those deltas unmerged until a full cursor reset.
Useful? React with 👍 / 👎.
|
@codex review Validated latest head Checks run:
Unresolved blocker: full |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84704f947f
ℹ️ 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 routeKeyInRange(store.ExtractLegacyListUserKeyFromDelta(rawKey), routeStart, routeEnd) || | ||
| routeKeyInRange(store.ExtractListUserKey(rawKey), routeStart, routeEnd) |
There was a problem hiding this comment.
For real legacy list delta rows (!lst|meta|d|<len><user><ts><seq>), the fallback ExtractListUserKey treats the same bytes as a base list-meta key whose user key starts with d|.... In a migration whose route range includes that synthetic d| key (for example [d|, d})), this OR includes legacy deltas for unrelated list users, so those raw delta versions can be exported/imported into the wrong shard in addition to the shard owning the decoded legacy user key. The base-meta fallback needs to be disambiguated from actual legacy delta rows, rather than accepting both route interpretations unconditionally.
Useful? React with 👍 / 👎.
| if startKey == nil { | ||
| return false |
There was a problem hiding this comment.
When callers pass StartKey: []byte{} with a non-empty EndKey, the logical range still includes the empty user key, but Pebble's encoded empty key sorts after ordinary non-empty keys. Because this stop check treats explicit empty start as safe to stop at the first userKey >= EndKey, ExportVersions returns Done before reaching a trailing empty-key version (e.g. ["", "b") with keys "" and "b" exports nothing). Treat an empty start like the unbounded case here so migrations do not drop empty user keys.
Useful? React with 👍 / 👎.
## Summary Author: bootjp Implements the M2-PR5 migration guards: - reject writes that target `WriteFenced` routes in the coordinator and FSM - reject `DEL_PREFIX` by route-footprint intersection, including full-range deletes - add a route-faithful transaction-lock drain helper that scans lock storage and filters by decoded route key - reject same-group `SplitRange` requests that overlap a live split job while allowing disjoint same-group splits ## Validation - `GOCACHE=$(pwd)/.cache GOTMPDIR=$(pwd)/.cache/tmp go test ./kv ./distribution ./adapter -run 'TestFSMRejects|TestShardedCoordinatorRejects|TestPendingTxnLocksInRoute|TestDistributionServerSplitRange_(RejectsLiveSplitJobOverlap|AllowsDisjointRouteWhileSplitJobLive|Success|UsesCoordinatorForCatalogWrites)|TestPlanMigrationBrackets|TestMigrationKnownInternalPrefixes|TestValidateMigrationRouteRange|TestSplitJobPlanner|TestVerifyComposed1|TestKvFSM' -count=1 -timeout=240s` - `GOCACHE=$(pwd)/.cache GOTMPDIR=$(pwd)/.cache/tmp go test ./kv ./distribution -count=1 -timeout=240s` - `GOCACHE=$(pwd)/.cache GOLANGCI_LINT_CACHE=$(pwd)/.golangci-cache golangci-lint run ./kv ./distribution ./adapter --timeout=5m`
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 (549a786):
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. |
1 participant
Summary
Tests
Author: bootjp