distribution: add migration wire contracts#1084
Conversation
|
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:
📝 WalkthroughWalkthroughChangesルート移行メタデータ
読み取りフェンス付きルーティング
Redisキー正規化
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GRPCServer
participant LeaderRoutedStore
participant ShardStore
Client->>GRPCServer: RawScanAt(read route version, route bounds)
GRPCServer->>LeaderRoutedStore: ScanAtWithReadFence
LeaderRoutedStore->>ShardStore: 境界付きローカルスキャンまたはRaw RPC
ShardStore-->>LeaderRoutedStore: フェンス済みKV結果
LeaderRoutedStore-->>GRPCServer: ScanAt結果
GRPCServer-->>Client: RawScanAt応答
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
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 (72fcc6b):
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. |
![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 introduces a V2 codec version for route descriptors in the distribution catalog to support new migration-related fields, including staged visibility, migration job IDs, and exclusive write timestamps. It also updates protobuf definitions and generated gRPC code to add several new RPC methods (such as split migration and range version export/import) and includes read route versions for migration read fences. The review feedback suggests defining an explicit catalogRouteCodecVersionV2 constant instead of using the dynamic catalogRouteCodecVersion directly in encoding and decoding paths, which prevents future codec version bumps from breaking V2-specific logic.
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 (7dd2e6d):
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: 7dd2e6d48a
ℹ️ 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".
|
@codex review Addressed current-head blockers in ef2ba76:
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 (ef2ba76):
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef2ba76416
ℹ️ 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".
| var err error | ||
| if groupID := req.GetGroupId(); groupID != 0 { | ||
| if fenceGetter, ok := r.store.(rawReadFenceGetter); ok { | ||
| v, err = fenceGetter.GetAtWithReadFence(ctx, req.Key, readTS, req.GetGroupId(), req.GetReadRouteVersion()) |
There was a problem hiding this comment.
External RawKV callers leave read_route_version unset, but this branch passes that zero straight into the fenced store path instead of stamping the catalog version observed while routing the request. When this server is a follower and proxies the read during a split migration, the internal RawGet still carries version 0, so the source-side cutover fence either treats a fresh routed read as a broken legacy hop once zero-version rejection is active or cannot distinguish it from a stale coordinator request; stamp the current route/catalog version before invoking the fence helper.
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 (2388b3e):
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 the current-head blockers in 2388b3e:
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: 2388b3e665
ℹ️ 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".
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 (781dc36):
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 the current-head fenced raw scan findings in 781dc36:
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: 781dc362ec
ℹ️ 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".
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 (1afde8f):
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. |
|
Fixed the two RawScanAt fence findings:
Caller audit:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1afde8f1ba
ℹ️ 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 reverse { | ||
| kvs, err := s.local.ReverseScanAt(ctx, start, end, limit, max(ts, fenceTS)) | ||
| return kvs, errors.WithStack(err) | ||
| } | ||
| kvs, err := s.local.ScanAt(ctx, start, end, limit, max(ts, fenceTS)) |
There was a problem hiding this comment.
When this store is the verified local leader and a RawScanAt/ScanAtWithReadFence call supplies routeStart/routeEnd, these branches read the local MVCC store directly and never apply the route-key filter/pagination that the ShardStore path uses. In a leader-routed deployment receiving a supplied-bound scan over an internal raw prefix, rows from same-group sibling route intervals can be returned or consume the page limit, while the non-leader proxy path preserves the bounds.
Useful? React with 👍 / 👎.
| scanEnd := end | ||
| for len(out) < limit { | ||
| batchLimit := routeFilteredScanBatchLimit(limit - len(out)) | ||
| kvs, err := s.scanRouteAtDirectionWithReadFenceOnce(ctx, route, scanStart, scanEnd, batchLimit, ts, reverse, explicitGroup, readRouteVersion, routeStart, routeEnd) |
There was a problem hiding this comment.
For supplied-bound scans on a local ShardStore leader, this call resolves MVCC/txn locks for the raw batch before appendRouteFilteredKVs drops rows whose routeKey is outside routeStart/routeEnd. If a same-group sibling route has a pending lock under the scanned raw prefix, the bounded scan can return ErrTxnLocked or hit the lock budget even though that sibling row should be skipped by the route filter.
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 (33c793f):
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. |
|
Fixed the two current-head review findings. Summary:
Caller audit:
Validation:
Commit: 33c793f @codex review |
|
Addressed the latest current-head route-bounded local scan findings in 33c793f. Changes:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33c793f03d
ℹ️ 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".
| var groupID uint64 | ||
| if explicitGroup { | ||
| groupID = route.GroupID | ||
| } | ||
| kvs, err := s.proxyRawScanAt(ctx, g, start, end, limit, ts, reverse, groupID, readRouteVersion, routeStart, routeEnd) |
There was a problem hiding this comment.
When a supplied route-bound scan falls into this follower branch with explicitGroup == false, groupID remains zero, so the RawScanAt sent to this route's leader is re-routed there as a multi-route scan using the supplied route bounds. On nodes that host or know multiple shard groups, that leader can return sibling-group rows; the caller then scans those sibling routes again in scanRoutesAtWithReadFence, producing duplicates and applying the page limit to the wrong combined set. The proxied route-bounded request needs to stay scoped to route.GroupID instead of relying on the receiver to rediscover routes.
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 (a456f48):
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. |
707cc61 to
0b3b1d1
Compare
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 (0b3b1d1):
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. |
0b3b1d1 to
6b40846
Compare
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 (6b40846):
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. |
6b40846 to
644fd08
Compare
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 (644fd08):
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. |
|
Current HEAD: 対応内容:
Caller audit:
Validation:
@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: 644fd08e81
ℹ️ 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".
| userKeyLen := binary.BigEndian.Uint32(trimmed[:wideColKeyLenSize]) | ||
| if uint32(len(trimmed)) < uint32(wideColKeyLenSize)+userKeyLen { //nolint:gosec // wideColKeyLenSize and encoded lengths fit in uint32 |
There was a problem hiding this comment.
If a caller supplies a malformed list delta/claim scan cursor whose encoded user-key length is near math.MaxUint32, the uint32(wideColKeyLenSize)+userKeyLen addition wraps, so the bounds check can pass even though the slice is too short; the subsequent slice on the next line then panics before the request can fail cleanly. This new scan-key parser is reachable from sharded scan routing for list auxiliary prefixes, so it should use a non-wrapping uint64/int length check like the stream helpers do.
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 (6c6bde4):
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. |
|
Current HEAD: 対応内容:
Caller audit:
Validation:
@codex review |
|
@codex review Latest-head review threads were rechecked against 6c6bde4 and resolved after verifying the fixes are present:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c6bde44ea
ℹ️ 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 *ShardStore) ensurePrefixWriteAllowed(prefix []byte, commitTS uint64) error { | ||
| routes := s.engine.GetIntersectingRoutes(prefix, prefixScanEnd(prefix)) |
There was a problem hiding this comment.
For raw internal prefixes such as the Redis FLUSHDB families (!lst|, !hs|, !st|, !zs|), this lookup checks the raw byte range instead of the logical user-key ranges those rows route by. A route fenced at, for example, Start: "m" is not inspected when deleting !hs|..., yet the subsequent delete still broadcasts tombstones to every group, so a commitTS <= MinWriteTSExclusive can be accepted for fenced logical keys.
Useful? React with 👍 / 👎.
| defer cancel() | ||
| cli := pb.NewRawKVClient(conn) | ||
| resp, err := cli.RawLatestCommitTS(ctx, &pb.RawLatestCommitTSRequest{Key: key}) | ||
| resp, err := cli.RawLatestCommitTS(ctx, &pb.RawLatestCommitTSRequest{Key: key, ReadRouteVersion: readRouteVersion}) |
There was a problem hiding this comment.
When the primary logical wide-column route is remote, this request still identifies only the key/version, so the leader answers RawLatestCommitTS with ShardStore.LatestCommitTSWithReadFence across both logical and legacy routes instead of just route.GroupID. If the logical route has a tombstone visible at the read timestamp but the legacy route has a newer future version, the fallback check does not stop and GetAt can return the older legacy value, resurrecting data that the logical tombstone should hide.
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 (83ac727):
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 the current wide-column migration read-fence findings in 83ac727:
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: 83ac727755
ℹ️ 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".
|
@codex review Addressed current-head review blockers in b6ff276:
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 (b6ff276):
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: b6ff276e13
ℹ️ 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 rawScanCanUseExplicitGroupReverse(req) { | ||
| if _, ok := r.store.(rawGroupReverseScanner); ok { | ||
| return r.rawScanAtExplicitGroup(ctx, req, req.GetGroupId(), limit, readTS) |
There was a problem hiding this comment.
When the backing store is a real ShardStore (it implements both rawGroupReverseScanner and rawReadFenceScanner), a RawScanAt request with group_id, reverse=true, and no route bounds takes this legacy explicit-group branch before the read-fence-aware path. That bypasses both the InvalidArgument rejection immediately below and server-side read_route_version stamping, so follower/proxy reverse scans can read an explicitly selected group without the migration fence that the value/key paths are trying to enforce. Apply the legacy shortcut only for stores that are not read-fence-aware, or let rawScanAtWithReadFence decide.
Useful? React with 👍 / 👎.
| latest, exists, err := s.latestCommitTSForRoute(ctx, route, key, readRouteVersion) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| return exists && latest <= ts, nil |
There was a problem hiding this comment.
For a non-leader primary route, this fallback probe only asks the leader for the latest commit timestamp; if the key has an older tombstone at or before ts and a later version after ts, latest > ts makes this return false and the code falls through to the legacy wide-column route. A snapshot read between those two timestamps can then resurrect the legacy value even though the primary route's tombstone should hide it. The remote probe needs to answer “any version at or before the read timestamp” rather than comparing only the newest commit timestamp.
Useful? React with 👍 / 👎.
| muts := grouped[gid] | ||
| if ts != 0 { | ||
| if err := c.ensureMutationsWriteAllowed(muts, ts); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
When c.tsAllocator is configured, rawLogTimestamp returns 0, so this branch skips MinWriteTSExclusive validation until leaseRefreshingTxn stamps the request. That works for local test/fake groups, but in a real follower path stampRawTimestampsForLocalCommit returns without stamping because the group engine is not leader, then Internal.Forward on the leader assigns r.Ts in adapter/internal.go and commits through a bare TransactionManager with no route-floor check. Non-transactional writes sent to a follower can therefore reach Raft at or below the route floor; the floor check needs to run on the forwarded stamping path too.
Useful? React with 👍 / 👎.
| legacy, legacyVersion, ok := s.engine.GetRouteWithVersion(legacyKey) | ||
| version = max(version, legacyVersion) |
There was a problem hiding this comment.
This second route lookup can observe a newer catalog version than the primary route read just above, and the code then forwards both route choices using the max version. If a Redis wide-column key's primary route moves between the two GetRouteWithVersion calls, the request can carry the newer read_route_version while still explicitly reading the stale primary group, which defeats the fence on the leader after it waits for that version. Fetch the primary and legacy route candidates from a single engine snapshot or retry when the versions differ.
Useful? React with 👍 / 👎.
| func normalizeRouteFilterKey(key []byte) []byte { | ||
| if user := redisListAuxiliaryRouteKey(key); user != nil { | ||
| return user | ||
| } | ||
| if user := redisStreamRouteKey(key); user != nil { | ||
| return user | ||
| } |
There was a problem hiding this comment.
This makes route-bound scans classify list-delta/claim and stream rows by their logical Redis key, but write admission still calls routeKey, which does not apply these normalizers (stream keys remain under !stream|..., and list deltas are treated as the raw !lst|meta|d|... suffix). When a logical key in a moved or fenced route has one of these auxiliary writes, the write is checked against the raw-prefix route instead of the user-key route, so it can bypass MinWriteTSExclusive and later route-bound export/scans for the user interval can miss the row. Apply the same decoders to write routing/floor checks, or stop using them as route-bound ownership filters.
Useful? React with 👍 / 👎.
| if scanned >= limit && !done { | ||
| return nil, errors.Wrapf(ErrTxnLocked, "scan lock budget exceeded for range [%q,%q)", string(start), string(end)) |
There was a problem hiding this comment.
For route-bound scans, this budget is incremented before the locks are filtered by routeKeyInScanBounds, so a page containing only locks for other logical routes can trip ErrTxnLocked as soon as scanned >= limit even though no relevant lock was found. A migrated-range scan over a raw Redis/list/stream prefix can therefore fail just because earlier keys in the same physical lock range belong to another route; the budget should be based on accepted route-matching locks or use a separate raw-scan cap large enough to page past unrelated locks.
Useful? React with 👍 / 👎.
1 participant
Summary
Tests
Note: full adapter-package testing exceeded the local timeout in existing raft/gRPC integration coverage; the targeted distribution adapter tests above passed.
Author: bootjp
Summary by CodeRabbit