store: add migration version import export - #1085
Conversation
|
Important Review skippedToo many files! This PR contains 116 files, which is 16 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (116)
You can disable this status message by setting the 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 👍 / 👎.
|
Merged One conflict: The auto-merge also produced a duplicate
|
|
Claude finished @bootjp's task in 2m 31s —— View job Review of the
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d77f07f542
ℹ️ 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 rawCopy(dst net.Conn, src io.Reader, done chan<- struct{}) { | ||
| _, _ = io.Copy(dst, src) | ||
| _ = dst.Close() |
There was a problem hiding this comment.
When a client sends a request and then half-closes its write side with shutdown(SHUT_WR), the client-to-upstream io.Copy reaches EOF and this closes the entire upstream connection; handleRawRedisConn then returns on that copier and closes the client as well. The reverse copier therefore cannot deliver the Redis response, breaking valid one-shot or half-closing TCP clients. Half-close only the destination's write side and allow the response-side copier to finish before closing both connections.
Useful? React with 👍 / 👎.
| if opts.MaxVersions <= 0 { | ||
| return ExportVersionsResult{Done: true}, nil |
There was a problem hiding this comment.
When MaxVersions is left at its zero value or configured negative, this reports Done: true without inspecting the requested range. A migration driver can therefore durably mark a non-empty bracket complete and proceed to cutover without copying any of its versions. Treat a non-positive budget as invalid, or apply a positive default, rather than using the completion signal.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Both backends answered Done for MaxVersions <= 0 without looking at the range, so a driver that left the field unset would record a non-empty bracket as fully copied and move on to cutover having moved nothing. A completion signal must never be the answer to an empty budget. Return ErrInvalidExportBudget instead. The adapter always sets the field (defaultMigrationExportMaxVersions), so no production caller changes behaviour; this closes the shape a future caller could fall into. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
Each copier closed the whole destination connection when its direction hit EOF, and handleRawRedisConn returned on the first copier to finish. A client that writes a command and then calls shutdown(SHUT_WR) -- valid TCP, and what one-shot clients do -- therefore had its upstream connection closed before the reply could be relayed, and its own connection closed right after. Shut down only that direction's write side, and wait for both directions before the deferred closes run. Transports without CloseWrite still take the full close so they cannot hang. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
Two of the three at-HEAD P2s fixed. HEAD is now
|
|
Claude finished @bootjp's task in 3m 7s —— View job Review of HEAD
|
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 (54d45ed):
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: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
# Conflicts: # docs/design/2026_05_28_implemented_tla_safety_spec.md
|
Resolved the current merge conflict and pushed the updated branch. Validation on the final head:
@codex review |
|
追加のレビュー対応を push しました。 変更内容:
検証:
|
…ct-fix # Conflicts: # kv/txn_keys.go
|
最新の 解決内容:
検証:
|
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 (dcab263):
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: dcab263bca
ℹ️ 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 (s *CatalogStore) retireSplitJobMigrationMetadata(ctx context.Context, jobID uint64) error { | ||
| if err := s.store.RetireMigration(ctx, jobID); err != nil && !errors.Is(err, store.ErrNotSupported) { |
There was a problem hiding this comment.
For a cross-group split whose TargetGroupID differs from the group owning the distribution catalog, ImportVersions acknowledgements and the HLC floor reside in the target group's store, while production constructs CatalogStore from the catalog group's runtime. Calling s.store.RetireMigration here therefore cleans the catalog store rather than the target store, leaving the target's metadata to accumulate indefinitely; the new tests miss this because they import and catalog against the same store. Route retirement to the completed job's target group.
Useful? React with 👍 / 👎.
|
current-head review の2件に対応しました。 変更内容:
検証:
|
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 (d2d620d):
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. Hooray! 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