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 👍 / 👎.
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65e2973b7e
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
kv/shard_key.go (1)
98-114: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winスライスの再割り当てを防ぐため、プレフィックスをパッケージレベル変数として定義する
現在の実装では、スキャンのルーティング処理が呼び出されるたびに、
[][]byteや内部の[]byteスライスが新しく割り当てられています。これをパッケージレベルの変数として切り出すことで、スキャン実行時の不要なアロケーションを削減し、パフォーマンスを改善できます。♻️ 提案するリファクタリング
+var wideColumnScanPrefixes = [][]byte{ + []byte(store.HashMetaDeltaPrefix), + []byte(store.HashMetaPrefix), + []byte(store.HashFieldPrefix), + []byte(store.SetMetaDeltaPrefix), + []byte(store.SetMetaPrefix), + []byte(store.SetMemberPrefix), + []byte(store.ZSetMetaDeltaPrefix), + []byte(store.ZSetMetaPrefix), + []byte(store.ZSetMemberPrefix), + []byte(store.ZSetScorePrefix), +} + func redisWideColumnScanRouteKey(key []byte) []byte { - for _, prefix := range [][]byte{ - []byte(store.HashMetaDeltaPrefix), - []byte(store.HashMetaPrefix), - []byte(store.HashFieldPrefix), - []byte(store.SetMetaDeltaPrefix), - []byte(store.SetMetaPrefix), - []byte(store.SetMemberPrefix), - []byte(store.ZSetMetaDeltaPrefix), - []byte(store.ZSetMetaPrefix), - []byte(store.ZSetMemberPrefix), - []byte(store.ZSetScorePrefix), - } { + for _, prefix := range wideColumnScanPrefixes { if user := wideColumnScanUserKey(key, prefix); user != nil { return user }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kv/shard_key.go` around lines 98 - 114, Update redisWideColumnScanRouteKey to reuse a package-level prefix collection instead of constructing the [][]byte and []byte values on every call. Define the prefixes once using the existing store prefix constants, then iterate over that shared variable while preserving the current wideColumnScanUserKey matching order and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@kv/shard_key.go`:
- Around line 126-130: Remove the //nolint directive in the key-decoding logic
and refactor the slice handling around keyLen: advance rest past
wideColumnEncodedKeyLengthSize first, validate that the remaining length can
contain keyLen, then slice using the simplified offsets without uint32 casts or
added variables.
In `@kv/shard_store_txn_lock_test.go`:
- Line 360: テストで生成した MVCCStore リソースを確実に解放する。kv/shard_store_txn_lock_test.go
の該当テストでは st に対する t.Cleanup を追加し、同テスト内の st1 と shardStore
も同様にクローズする。kv/leader_routed_store_test.go では store.NewMVCCStore() の結果を変数へ抽出し、既存の
t.Cleanup 内で s.Close() とともにクローズする。
- Around line 373-374: Update the routeStart argument in the BoundsMatchingLocks
test call to scanTxnLockPagesAtWithRouteFilter so the generated
!redis|meta|z0000 keys fall within the route bounds, while keeping the routeEnd
and other arguments unchanged. Ensure the test exercises the intended
matching-locks path where the collected locks exceed lockPageLimit.
---
Nitpick comments:
In `@kv/shard_key.go`:
- Around line 98-114: Update redisWideColumnScanRouteKey to reuse a
package-level prefix collection instead of constructing the [][]byte and []byte
values on every call. Define the prefixes once using the existing store prefix
constants, then iterate over that shared variable while preserving the current
wideColumnScanUserKey matching order and return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6d8646e-c6fb-4289-9e71-38467e08c07f
📒 Files selected for processing (9)
Makefileadapter/grpc.goadapter/grpc_test.gokv/leader_routed_store.gokv/leader_routed_store_test.gokv/shard_key.gokv/shard_store.gokv/shard_store_test.gokv/shard_store_txn_lock_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- Makefile
- kv/shard_store_test.go
- kv/leader_routed_store.go
- adapter/grpc.go
- kv/shard_store.go
- adapter/grpc_test.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 (bd17534):
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\n\nAddressed the current-head read-routing findings in bd17534:\n- atomically paired route selection with its catalog version for scans and point reads\n- preserved selected groups for unclamped fan-out scans\n- restored deterministic ordering after route-filtered lock resolution\n- tightened wide-column key decoding and test cleanup/coverage\n\nCaller audit:\n- GetAt/GetAtWithReadFence and LatestCommitTS/LatestCommitTSWithReadFence select and forward the same route snapshot version\n- forward, reverse, and keys-only scan fan-out paths preserve explicit groups only where the outer loop owns fan-out\n- explicit-group receiver paths retain the supplied read fence\n\nValidation:\n- go test ./distribution ./kv -count=1\n- go test ./... -run '^$' -count=1\n- golangci-lint run ./distribution/... ./kv/... --timeout=5m\n- git diff --check |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd1753406c
ℹ️ 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 (2f73906):
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 Latest HEAD 2f73906 addresses both current-head findings: unsupported value read-fence requests now fail closed, and broad Redis wide-column prefix scans fan out over the decoded logical route interval while exact collection scans remain single-route. Validation: targeted adapter/kv tests passed; go test ./kv -count=1 passed; go test ./... -run ^$ -count=1 passed; golangci-lint run ./... --timeout=5m passed. The full adapter integration suite timed out at its existing 10-minute limit under concurrent local test load; the affected adapter tests passed separately. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f739063fa
ℹ️ 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 (6bebfda):
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. |
|
Review fixes are available at 6bebfda.\n\n- RawGet and RawLatestCommitTS now fail closed when a requested route fence cannot be enforced.\n- Bare Redis wide-column namespace scans and physical cursor scans fan out across all logical routes; exact collection scans remain single-route.\n- The current main conflict is resolved.\n- Catalog V2 decoding rejects trailing bytes after the fixed-size tail.\n\nEvidence:\n- go test ./distribution ./kv .\n- focused adapter read-fence tests\n- golangci-lint run ./adapter ./distribution ./kv --timeout=5m --allow-parallel-runners: 0 issues\n- make gen: no working-tree diff\n- every commit in origin/main..6bebfda has author and committer bootjp contact@bootjp.me and a valid signature\n\n@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bebfda6bc
ℹ️ 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".
| encoded, err := distribution.EncodeRouteDescriptorForCatalogWrite(route, allowRouteDescriptorV2Writes) | ||
| if err != nil { | ||
| return nil, errors.WithStack(err) | ||
| } |
There was a problem hiding this comment.
When the route being split carries any migration extension field such as MinWriteTSExclusive, this encoder emits the v3 route layout, where SplitAtHLC is no longer the final 8 bytes of the descriptor. The following splitAtHLCPatchOffset call still assumes the last word is SplitAtHLC, so the coordinator stamps the split commit timestamp over MinWriteTSExclusive and leaves SplitAtHLC at zero. Splitting a write-fenced/migrating route will therefore corrupt the catalog fence metadata; compute the patch offset from the encoded version/layout rather than assuming the final word.
Useful? React with 👍 / 👎.
| if ok { | ||
| val, err := s.local.GetAt(ctx, key, max(ts, fenceTS)) | ||
| return val, errors.WithStack(err) |
There was a problem hiding this comment.
When a RawKV server is backed by LeaderRoutedStore and a caller supplies a non-zero ReadRouteVersion, GRPCServer dispatches here because this type now satisfies the read-fence getter interface. The local-leader branch ignores that requested catalog fence and returns local data after only a leadership fence; the same pattern exists in the latest-commit and scan read-fence helpers. During migration this can silently satisfy a fenced read without proving the node has the requested route catalog version, so this wrapper should fail closed for non-zero readRouteVersion unless it can actually enforce it.
Useful? React with 👍 / 👎.
|
|
||
| func (s *ShardStore) routesForFilesystemChunkScan(start []byte, end []byte) ([]distribution.Route, bool) { | ||
| func (s *ShardStore) routesForFilesystemChunkScanWithVersion(start []byte, end []byte) ([]distribution.Route, uint64, bool) { | ||
| allRoutes, version := s.engine.GetIntersectingRoutesWithVersion(nil, nil) |
There was a problem hiding this comment.
For scans that are not filesystem chunk scans, this eagerly clones the entire route table before the function checks whether the requested range even overlaps the filesystem chunk keyspace, and the returned version is discarded on the !ok path. With many split routes, ordinary Redis/Dynamo/raw scans pay an extra O(number of routes) allocation and scan before doing their actual routing lookup; defer the all-routes snapshot until after ChunkScanRouteBounds or filesystemChunkScanOverlap proves it is needed.
Useful? React with 👍 / 👎.
| if user := redisWideColumnRouteKey(key); user != nil { | ||
| return user |
There was a problem hiding this comment.
When upgrading a sharded cluster that already has Redis hash/set/zset rows written before this change, those rows were placed by the raw storage key while this new normalization sends exact reads and later writes to the route owning the logical user key. If the catalog has a boundary between the raw internal prefixes and a user key, for example !hs|... in group 1 but user key z in group 2, exact HGET/HSCAN/SISMEMBER/ZRANGE-style paths remain single-route and will only contact group 2, leaving the existing rows stranded on group 1. Add a compatibility read/migration path or gate the routing change until old wide-column data has been moved.
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 (94bf4cd):
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. |
|
Fixed the current-head race CI failure in 94bf4cd. The V3 route codec appends SplitAtHLC before staged/job/write-fence fields, but the previous patch offset used the final 8 bytes and overwrote MinWriteTSExclusive. The codec encoder now returns the exact SplitAtHLC offset while constructing V2/V3 records; the existing write fence remains monotone. Evidence:
|
|
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: 94bf4cd42c
ℹ️ 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 (707cc61):
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: Resolved the current P1 list auxiliary scan routing finding and the current-main conflict:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 707cc6178b
ℹ️ 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 userKey == nil { | ||
| return nil, 0, false |
There was a problem hiding this comment.
When the sharded Redis delta compactor runs its background list pass, scanDeltaRaw starts at the bare store.ListMetaDeltaPrefix, not a per-user scan prefix. For that valid all-list-delta prefix listScanUserKey returns nil and this branch opts out of encoded routing, so routesForScanWithVersion falls back to the raw !lst|meta|d| interval instead of fanning out to all route groups like the hash/set/zset bare-family path. In a split catalog, deltas for users outside that raw interval are not scanned/compacted by the background pass, leaving hot lists dependent on urgent repair.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func routeKeyInScanBounds(key []byte, routeStart []byte, routeEnd []byte) bool { | ||
| key = routeKey(key) |
There was a problem hiding this comment.
Route-bound scans use this predicate for migration/fenced export filtering, but routeKey() still lacks concrete decoders for Redis stream meta/entry keys and list delta/claim keys (list deltas are swallowed by the broad ListMetaPrefix case as d|...). When a moving route contains one of those user keys, the KV is compared using its raw/internal prefix rather than the logical Redis key and is filtered out of the route-bounded page. That drops stream rows or list auxiliary rows from staged reads/export for the range; add those prefix decoders before relying on routeKey() here.
Useful? React with 👍 / 👎.
| // MinWriteTSExclusive rejects writes at or below the migration cutover floor. | ||
| MinWriteTSExclusive uint64 |
There was a problem hiding this comment.
When a catalog snapshot contains a route with MinWriteTSExclusive (for example on the target after cutover), the field is carried into the in-memory route but the write paths still only resolve a group and then call ApplyMutations* without checking the caller-supplied commitTS against the floor. A raw/one-phase/prepare write at commitTS <= MinWriteTSExclusive can therefore create a live version that sorts below the migrated versions and is hidden by reads, reopening the cutover data-loss case this descriptor field is meant to prevent.
Useful? React with 👍 / 👎.
| if req.GetGroupId() != 0 && req.GetReverse() && !req.GetRouteBoundsPresent() { | ||
| return nil, errors.WithStack(status.Error(codes.InvalidArgument, "raw scan with explicit group does not support reverse scans")) |
There was a problem hiding this comment.
When a follower handles an unbounded/non-clamped ReverseScanAt (for example start/end nil), ShardStore proxies each backing group with group_id set and no route bounds; before this change the explicit-group reverse path handled that. Since the read-fence branch now rejects group_id + reverse unless route bounds are present, a remote leader backed by ShardStore returns InvalidArgument and the scan fails instead of returning data. Route-bound grouped reverse remains covered, but ordinary non-clamped reverse still needs the explicit-group path.
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