distribution: add migration wire contracts#1084
Conversation
📝 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応答
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 👍 / 👎.
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. |
|
Addressed the current-head route-bound scan proxy finding in a456f48. Changes:
Validation:
Caller audit:
@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: a456f48092
ℹ️ 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".
|
Addressed the latest current-head route-bound scan findings in bcdee5b. Changes:
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 (bcdee5b):
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. |
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcdee5b5d8
ℹ️ 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 (5994c04):
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 route-bound grouped reverse scan blocker in 5994c04. Changes:
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: 5994c0428a
ℹ️ 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 err != nil { | ||
| return nil, nil, errors.WithStack(err) | ||
| } | ||
| lockStart, lockEnd := scanLockBoundsForKVs(kvs, start, end, limit) |
There was a problem hiding this comment.
When this route-filtered path is used for the newly allowed explicit-group reverse scans, this call still computes forward scan lock bounds: for a reverse page containing the highest key (e.g. z) with limit=1, it scans transaction locks from start through z rather than only the reverse page/window. A pending lock on a lower key that would not affect the first reverse result can therefore make the scan return ErrTxnLocked, so route-bound grouped reverse scans can fail incorrectly whenever older/lower keys in the same route are locked.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| out = appendRouteFilteredKVs(out, kvs, limit, routeStart, routeEnd) |
There was a problem hiding this comment.
When supplied route bounds span multiple current route descriptors, each group scan is filtered only by the overall routeStart/routeEnd, not by the specific descriptor being scanned. For an internal-prefix scan over [a,z) after a split such as group1 [a,m) and group2 [m,z), group1 is allowed to return stale raw rows whose normalized route key is in [m,z) because those keys still pass this global filter, so migrated/out-of-route data can leak into route-bound scan results.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
adapter/grpc.go (1)
117-127: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win明示的 GroupId 指定時のエラーステータスが後退している。
rawReadFenceGetter/rawReadFenceScannerを実装するストア(例:LeaderRoutedStore)が優先されるようになったため、GroupId != 0のケースでも従来の「group-aware store が必要」というcodes.FailedPrecondition判定を経由しなくなりました。LeaderRoutedStore.GetAtWithReadFence/ScanAtWithReadFenceはgroupID != 0のとき素のstore.ErrNotSupportedを返すため、errors.WithStack(err)でラップされるだけとなり、クライアントにはcodes.Unknown相当の不明瞭なエラーが返ります。
GroupId != 0の場合はフェンス経路に渡す前に group サポートの有無を確認し、非対応なら以前と同じFailedPreconditionを返すよう分岐順序を調整することを推奨します。🐛 修正案 (RawGet)
var v []byte var err error - if fenceGetter, ok := r.store.(rawReadFenceGetter); ok { + groupID := req.GetGroupId() + if fenceGetter, ok := r.store.(rawReadFenceGetter); ok && (groupID == 0 || supportsExplicitGroup(r.store)) { v, err = fenceGetter.GetAtWithReadFence(ctx, req.Key, readTS, req.GetGroupId(), r.readRouteVersion(req.GetReadRouteVersion())) - } else if groupID := req.GetGroupId(); groupID != 0 { + } else if groupID != 0 { groupGetter, ok := r.store.(rawGroupGetter) if !ok { return nil, errors.WithStack(status.Error(codes.FailedPrecondition, "raw get with explicit group requires a group-aware store")) } v, err = groupGetter.GetGroupAt(ctx, groupID, req.Key, readTS) } else { v, err = r.store.GetAt(ctx, req.Key, readTS) }(
rawScanAtにも同様の判定を追加してください)Also applies to: 198-224
🤖 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 `@adapter/grpc.go` around lines 117 - 127, Update the RawGet branching around rawReadFenceGetter and the corresponding rawScanAt path so explicit nonzero GroupId requests validate rawGroupGetter support before entering the read-fence route. Return the existing FailedPrecondition status when group support is unavailable, while preserving fence handling for supported stores and current behavior for requests without an explicit group.kv/leader_routed_store.go (1)
77-99: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winLeaderRoutedStore の proxy 系 read-fence 呼び出しに context timeout が無い。
proxyRawGetWithReadFence/proxyRawLatestCommitTSWithReadFence/proxyRawScanAtWithReadFenceはいずれも呼び出し元のctxをそのまま gRPC 呼び出しに使用しており、context.WithTimeoutが設定されていません。リモートリーダーが応答しない場合、呼び出しが無期限にブロックし続けます。同じ
kvパッケージのkv/shard_store.goにあるproxyRawGet/proxyLatestCommitTS/proxyRawScanAtはcontext.WithTimeout(ctx, proxyForwardTimeout)を用いて同種のリスクに対処しています。同じ回避策をここにも適用してください。🔒️ 修正案(3関数共通のパターン)
func (s *LeaderRoutedStore) proxyRawGetWithReadFence(ctx context.Context, key []byte, ts uint64, readRouteVersion uint64) ([]byte, error) { addr := s.leaderAddrForKey(key) if addr == "" { return nil, errors.WithStack(ErrLeaderNotFound) } conn, err := s.connCache.ConnFor(addr) if err != nil { return nil, err } + ctx, cancel := context.WithTimeout(ctx, proxyForwardTimeout) + defer cancel() cli := pb.NewRawKVClient(conn) resp, err := cli.RawGet(ctx, &pb.RawGetRequest{Key: key, Ts: ts, ReadRouteVersion: readRouteVersion})(
proxyRawLatestCommitTSWithReadFenceとproxyRawScanAtWithReadFenceにも同様に追加)Also applies to: 120-137, 161-206
🤖 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/leader_routed_store.go` around lines 77 - 99, LeaderRoutedStore の read-fence 付き proxy 呼び出しが無期限に待機しないよう、proxyRawGetWithReadFence、proxyRawLatestCommitTSWithReadFence、proxyRawScanAtWithReadFence の各 gRPC 呼び出し前に proxyForwardTimeout を使った context.WithTimeout を設定し、生成した context を呼び出しと適切な解放処理に使用してください。
🧹 Nitpick comments (2)
distribution/catalog.go (1)
785-822: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff新規追加のエラーラップが
pkg/errorsを使用しています（ガイドライン上はcockroachdb/errors）。
decodeRouteDescriptorTail・decodeRouteDescriptorV2Tailはerrors.WithStack/errors.Wrapf（github.com/pkg/errors）でラップしています。コーディングガイドラインでは境界部のエラーはgithub.com/cockroachdb/errorsでラップすることが求められています。ただし本ファイル全体が既にpkg/errorsで統一されている既存の傾向であるため、本PR単独での部分的な移行は同一ファイル内で2つの規約が混在する結果になりかねません。ファイル全体の移行が必要な広範な変更であることを踏まえて優先度を判断してください。
As per coding guidelines, "Wrap errors withgithub.com/cockroachdb/errorsat boundaries in Go code".🤖 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 `@distribution/catalog.go` around lines 785 - 822, Update decodeRouteDescriptorTail and decodeRouteDescriptorV2Tail to use github.com/cockroachdb/errors for boundary error wrapping, while migrating all existing error wrapping in distribution/catalog.go consistently so pkg/errors and cockroachdb/errors do not remain mixed. Preserve the current error conditions and messages.Source: Coding guidelines
distribution/catalog_test.go (1)
172-215: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winデコード側での不整合な移行フィールド組み合わせのテスト追加を検討してください。
現行のテストは
EncodeRouteDescriptor経由（validateRouteDescriptorによりエンコード前に無効な組み合わせが弾かれる）のみを検証しています。DecodeRouteDescriptorが破損したV2テール（例:staged=1だがmigration_job_id=0）を読み込んだ際にErrCatalogInvalidRouteRecordを返すケースは未検証です。TestRouteDescriptorCodecRejectsV2TrailingBytesのように生バイト列を手動構築して検証を追加すると、永続化データの破損検知の信頼性が高まります。🤖 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 `@distribution/catalog_test.go` around lines 172 - 215, DecodeRouteDescriptorの破損V2テール検証を追加し、EncodeRouteDescriptorを経由せず生バイト列を手動構築して、staged=1かつmigration_job_id=0など移行フィールドの不整合な組み合わせを読み込ませるテストを作成してください。DecodeRouteDescriptorがErrCatalogInvalidRouteRecordを返すことを確認し、既存のTestRouteDescriptorCodecRejectsV2TrailingBytesと同様の検証形式にしてください。
🤖 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_store.go`:
- Around line 209-220: Update ScanAtWithReadFence to return early for limit <= 0
before entering the reverse/group/bounds route-scan branch, matching the guards
used by other scan paths and preventing negative-capacity allocation in route
filtering.
---
Outside diff comments:
In `@adapter/grpc.go`:
- Around line 117-127: Update the RawGet branching around rawReadFenceGetter and
the corresponding rawScanAt path so explicit nonzero GroupId requests validate
rawGroupGetter support before entering the read-fence route. Return the existing
FailedPrecondition status when group support is unavailable, while preserving
fence handling for supported stores and current behavior for requests without an
explicit group.
In `@kv/leader_routed_store.go`:
- Around line 77-99: LeaderRoutedStore の read-fence 付き proxy
呼び出しが無期限に待機しないよう、proxyRawGetWithReadFence、proxyRawLatestCommitTSWithReadFence、proxyRawScanAtWithReadFence
の各 gRPC 呼び出し前に proxyForwardTimeout を使った context.WithTimeout を設定し、生成した context
を呼び出しと適切な解放処理に使用してください。
---
Nitpick comments:
In `@distribution/catalog_test.go`:
- Around line 172-215:
DecodeRouteDescriptorの破損V2テール検証を追加し、EncodeRouteDescriptorを経由せず生バイト列を手動構築して、staged=1かつmigration_job_id=0など移行フィールドの不整合な組み合わせを読み込ませるテストを作成してください。DecodeRouteDescriptorがErrCatalogInvalidRouteRecordを返すことを確認し、既存のTestRouteDescriptorCodecRejectsV2TrailingBytesと同様の検証形式にしてください。
In `@distribution/catalog.go`:
- Around line 785-822: Update decodeRouteDescriptorTail and
decodeRouteDescriptorV2Tail to use github.com/cockroachdb/errors for boundary
error wrapping, while migrating all existing error wrapping in
distribution/catalog.go consistently so pkg/errors and cockroachdb/errors do not
remain mixed. Preserve the current error conditions and messages.
🪄 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: 6b05f09e-f4a9-4b9c-8478-326612490eae
⛔ Files ignored due to path filters (5)
proto/distribution.pb.gois excluded by!**/*.pb.goproto/distribution_grpc.pb.gois excluded by!**/*.pb.goproto/internal.pb.gois excluded by!**/*.pb.goproto/internal_grpc.pb.gois excluded by!**/*.pb.goproto/service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (16)
adapter/distribution_server.goadapter/distribution_server_test.goadapter/grpc.goadapter/grpc_test.godistribution/catalog.godistribution/catalog_test.godistribution/engine.godistribution/engine_test.gokv/leader_routed_store.gokv/leader_routed_store_test.gokv/shard_store.gokv/shard_store_test.gokv/shard_store_txn_lock_test.goproto/distribution.protoproto/internal.protoproto/service.proto
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 (c88f654):
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: c88f654018
ℹ️ 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 nil, nil, errors.WithStack(err) | ||
| } | ||
| filteredKVs := filterRouteScanKVs(kvs, routeStart, routeEnd) | ||
| lockStart, lockEnd := scanLockBoundsForKVsDirection(filteredKVs, start, end, visibleLimit, reverse) |
There was a problem hiding this comment.
For route-bound scans whose raw storage page is full but filtering leaves fewer than visibleLimit in-route rows, this falls back to the original [start,end) lock range even though the caller is only processing the current raw page. If the scan later would find enough committed in-route rows before a farther pending lock, that farther lock is still scanned here and can make the bounded page return ErrTxnLocked incorrectly; compute the lock window from the raw page/cursor as well as the filtered rows.
Useful? React with 👍 / 👎.
| if _, seen := seenGroups[route.GroupID]; seen { | ||
| continue | ||
| } |
There was a problem hiding this comment.
When supplied route bounds span multiple descriptors that share a GroupID (for example after a same-group split), this de-duplication now skips the later descriptor after the first scan has already narrowed its route filter to only the first descriptor's interval. Keys whose normalized route key falls in the skipped sibling interval are therefore omitted from route-bound forward scans; the reverse path has the same seenGroups pattern and drops the opposite side. Since the current code filters by each descriptor before this skip, same-group routes need either separate descriptor scans or a combined per-group interval filter rather than skipping all later descriptors.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func routeKeyInScanBounds(key []byte, routeStart []byte, routeEnd []byte) bool { | ||
| key = routeKey(key) |
There was a problem hiding this comment.
For route-bound scans over the wide Redis families (!hs|, !st|, or !zs|), this filter compares the raw internal key because routeKey does not decode the hash/set/zset helper layouts. In a moving range such as [m,z), a row like !hs|meta|...z... sorts before m as raw bytes and is dropped, so migration/read-fence scans miss hash/set/zset state even though the embedded user key is in range; add those family extractors to the route-key normalization used here.
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 (0d130ad):
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 current-head route-filtered scan blockers in 0d130ad:
Caller audit:
Validation:
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
kv/shard_store_txn_lock_test.go (1)
421-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueコンテキストの再利用
402行目で宣言済みの
ctx変数があるため、新たにcontext.Background()を呼び出すのではなく、既存の変数を再利用することをお勧めします。♻️ 修正案
startTS := uint64(2) - _, err := groups[1].Txn.Commit(context.Background(), []*pb.Request{makePrepareRequest(startTS, left, []byte("left"), left)}) + _, err := groups[1].Txn.Commit(ctx, []*pb.Request{makePrepareRequest(startTS, left, []byte("left"), left)})🤖 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_store_txn_lock_test.go` around lines 421 - 422, Update the Txn.Commit call in the test to reuse the existing ctx variable declared earlier instead of creating a new context.Background() value; leave the surrounding transaction setup unchanged.kv/shard_store_test.go (1)
218-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffテーブル駆動テストへのリファクタリング（コーディングガイドライン）
コーディングガイドラインにて「
*_test.goではテーブル駆動テストの利用を推奨する」と規定されています。現状、両ファイルに渡って追加された複数のテストは独立した関数（フラットテスト）として実装されています。テストの重複コードを削減し保守性を高めるため、以下の箇所においてテーブル駆動テストへの統合をご検討ください。
kv/shard_store_test.go#L218-L254: これらの類似したScanAtWithReadFenceテストを1つのテーブル駆動テストに統合することをご検討ください。kv/shard_store_txn_lock_test.go#L357-L397: ロック境界の走査に関する一連のテストをテーブル駆動テストにまとめることをご検討ください。As per coding guidelines:
**/*_test.go: Unit tests should be co-located with packages in*_test.gofiles; prefer table-driven test cases.🤖 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_store_test.go` around lines 218 - 254, テストの重複を減らすため、kv/shard_store_test.go の218-254行にある TestShardStoreScanAtWithReadFence_ScansSameGroupSuppliedBoundsAcrossRouteIntervals を、入力条件・期待キー・逆順指定などをケース化したテーブル駆動テストへ統合してください。kv/shard_store_txn_lock_test.go の357-397行にあるロック境界走査テスト群も同様に1つのテーブル駆動テストへまとめ、各ケースの既存の期待結果と検証内容は維持してください。Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@kv/shard_store_test.go`:
- Around line 218-254: テストの重複を減らすため、kv/shard_store_test.go の218-254行にある
TestShardStoreScanAtWithReadFence_ScansSameGroupSuppliedBoundsAcrossRouteIntervals
を、入力条件・期待キー・逆順指定などをケース化したテーブル駆動テストへ統合してください。kv/shard_store_txn_lock_test.go
の357-397行にあるロック境界走査テスト群も同様に1つのテーブル駆動テストへまとめ、各ケースの既存の期待結果と検証内容は維持してください。
In `@kv/shard_store_txn_lock_test.go`:
- Around line 421-422: Update the Txn.Commit call in the test to reuse the
existing ctx variable declared earlier instead of creating a new
context.Background() value; leave the surrounding transaction setup unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b905ea6-55e7-46b7-8d7e-d484e764847f
📒 Files selected for processing (9)
adapter/distribution_server.goadapter/distribution_server_test.godistribution/catalog.godistribution/catalog_test.gokv/shard_key.gokv/shard_key_test.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 (3)
- adapter/distribution_server_test.go
- distribution/catalog_test.go
- kv/shard_store.go
|
Codex Review: Didn't find any major issues. Keep them coming! 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
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