backup: add live scan primitives#1065
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough変更概要指定時刻で可視なキーのみを取得するストアAPIを追加し、シャード横断のソート・重複排除・ルーティングを拡張しました。さらに、ページ単位で走査するバックアップスキャナと関連テストを追加しました。 Changesキー走査とバックアップスキャン
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BackupScanner
participant ShardStore
participant StoreOrProxy
Client->>BackupScanner: Next(ctx)
BackupScanner->>ShardStore: scanRoutesAtSorted(cursor, end, pageSize, ts)
ShardStore->>StoreOrProxy: ScanKeysAtまたはRawScanAt
StoreOrProxy-->>ShardStore: 走査結果
ShardStore-->>BackupScanner: ソート済みページ
BackupScanner-->>Client: KVPairまたは終了
🚥 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 (bd4a75c):
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. |
|
Ready for review. This implements the logical-backup scan primitive slice:
Validation:
|
![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 new BackupScanner for paging through ShardStore scans without holding locks, and implements a ScanKeysAt method across various store layers (LeaderRoutedStore, ShardStore, MVCCStore, and pebbleStore) to fetch only keys visible at a given timestamp. The review feedback highlights a critical bug in BackupScanner where an empty last key can trigger an infinite loop by resetting the cursor instead of closing the scanner. Additionally, several redundant bytes.Clone operations were identified across the newly added methods, which should be removed to avoid unnecessary memory allocations.
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.
|
Addressed the current-head scan review findings. 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 (a9f2581):
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: a9f258151e
ℹ️ 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 対応しました。
検証:
|
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 (4b85955):
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: 4b859556f9
ℹ️ 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 (582b2eb):
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 対応しました。
検証:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 582b2eba2e
ℹ️ 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 (aa99e90):
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 対応しました。
検証:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
kv/leader_routed_store.go (1)
263-287: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winproxy 分岐のキー抽出を
keysFromKVsに統一してください
手書きループだとkvp.Key == nilを落とせず、kv/shard_store.goの共通処理と挙動がずれます。return keysFromKVs(kvs), nilで十分です。🤖 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 263 - 287, Update LeaderRoutedStore.ScanKeysAt’s proxy branch to replace the manual kvp iteration with the shared keysFromKVs helper, returning its result directly with a nil error and preserving the existing proxy error handling.
🤖 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 634-734: Update the proxy branch of scanRouteAtForwardPage to
retain the unfiltered scan result separately, then filter only the returned kvs
while deriving advanceKey and full from the original raw page, matching the
local and leader branches. Preserve the existing proxy scan and filtering
behavior while ensuring pagination continues when raw results fill the requested
limit.
- Around line 295-339: The scanRoutesAt path must skip duplicate routes sharing
the same GroupID when clampToRoutes is false. Add seenGroups tracking and bypass
already-seen groups before calling scanRouteAt, matching scanKeyRoutesAt
behavior while preserving per-route scanning when clamping is enabled.
---
Nitpick comments:
In `@kv/leader_routed_store.go`:
- Around line 263-287: Update LeaderRoutedStore.ScanKeysAt’s proxy branch to
replace the manual kvp iteration with the shared keysFromKVs helper, returning
its result directly with a nil error and preserving the existing proxy error
handling.
🪄 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: 85010916-850a-4531-a052-aad914aa0d9f
📒 Files selected for processing (11)
kv/backup_scan.gokv/leader_routed_store.gokv/leader_routed_store_test.gokv/shard_store.gokv/shard_store_test.gokv/shard_store_txn_lock_test.gostore/lsm_store.gostore/lsm_store_encryption_test.gostore/mvcc_store.gostore/scan_keys_test.gostore/store.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa99e90709
ℹ️ 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 (9adece8):
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. |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
…anner # Conflicts: # kv/leader_routed_store.go # kv/shard_store.go # store/lsm_store.go # store/mvcc_store.go
|
Merged current main and resolved backup scanner scan conflicts. The resolved code keeps the main-branch physical-limit scan APIs and context checks, while preserving the live-backup ScanKeysAt / BackupScanner pagination fixes. 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 (6cc845d):
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. Breezy! 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". |
|
@codex review Current head checked: I rechecked the current-head top-level inline comments against the live code. No new code changes were needed; the current branch already contains the fixes for the latest-head review candidates:
Validation:
No commit was created because the worktree was already clean at the current PR head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cc845d01e
ℹ️ 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 *pebbleStore) collectScanKeys(ctx context.Context, iter *pebble.Iterator, start, end []byte, limit int, ts uint64) ([][]byte, error) { | ||
| result := make([][]byte, 0, boundedScanResultCapacity(limit)) | ||
|
|
||
| for iter.SeekGE(encodeKey(start, math.MaxUint64)); iter.Valid() && len(result) < limit; { |
There was a problem hiding this comment.
Because this new key-only scanner stops as soon as len(result) == limit while walking Pebble’s encoded-key order, it can return a later logical key before an earlier prefix key. User keys are allowed to contain arbitrary bytes; for example, with visible keys "\x00" and "\x00a", Pebble sorts the longer key first because the shorter key’s timestamp suffix compares after a, so ScanKeysAt(..., limit=1) returns "\x00a". ShardStore’s refill cursor then advances past "\x00", causing paginated key scans/backup counts to skip it. Please choose the first limit decoded user keys in logical byte order rather than truncating in physical iterator order.
Useful? React with 👍 / 👎.
1 participant
Author: bootjp
Summary
Validation
Summary by CodeRabbit