Add physical snapshot export substrate#1131
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPebble の SST ingest スナップショット転送、検証付き原子的復元、物理 FSM スナップショットのエクスポート API、関連テストと設計文書を追加・更新しています。 ChangesSST ingest スナップショット転送
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Raft
participant pebbleStore
participant GRPCTransport
participant destinationStore
Raft->>pebbleStore: Snapshot()
pebbleStore->>GRPCTransport: EKVSSTI1 payloadを送信
GRPCTransport->>destinationStore: receiveSnapshotStream()
destinationStore->>destinationStore: manifest検証・SST ingest・atomic swap
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 |
|
Current head: Evidence:
Validation:
Please review the persisted snapshot consistency boundary, single-use stream lifecycle, CRC failure behavior, complete-payload restore semantics, and the explicit dependency boundary with #1130. @codex review |
![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 the initial substrate for the physical snapshot object offload design (milestone M4). It adds a detailed design document, implements PreparePhysicalSnapshotRestore to restore fresh etcd-raft data directories from opaque FSM payloads, and provides OpenPersistedSnapshotExport to safely stream locally persisted snapshots. Feedback is provided regarding a potential data race in Metadata() if proto.Clone fails, where the original ConfState pointer could be leaked instead of being set to nil.
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.
|
Updated head: Resolved the metadata ownership finding by clearing Validation:
@codex review |
![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: 35479d5e8d
ℹ️ 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".
35479d5 to
9012ed1
Compare
|
Updated stacked head: Resolved both current-head restore membership findings:
Caller audit:
Validation:
@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: 1
🧹 Nitpick comments (2)
kv/snapshot_sst_ingest_test.go (1)
14-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTTL 付きキーの SST ingest スナップショット round-trip カバレッジを追加してください。
本テストは
kv/パッケージの Snapshot/Restore（replication 経路）に新たに触れていますが、PutAtの TTL 引数は常に0で、TTL 付きエントリが SST ingest スナップショット経由で正しく復元されるかは検証されていません。TTL 付きキーのケースを1つ追加することを推奨します。As per coding guidelines, "Add test coverage for TTL/HLC behaviors, OCC logic, and replication logic whenever touching clocks, optimistic concurrency control, or replication mechanisms" for
{store,kv}/**/*_test.go.♻️ 追加例
require.NoError(t, srcStore.PutAt(context.Background(), []byte("key"), []byte("value"), 41, 0)) + require.NoError(t, srcStore.PutAt(context.Background(), []byte("ttl-key"), []byte("ttl-value"), 41, 100))そして復元後に該当キーの TTL/期限が維持されていることを検証するアサーションを追加してください。
🤖 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/snapshot_sst_ingest_test.go` around lines 14 - 40, Test TestKVFSMSnapshotRoundTripsSSTIngestPayload with an additional key written via PutAt using a non-zero TTL, then include it in the SST ingest snapshot and restore flow. After Restore, assert both the TTL/expiration metadata and value for that key are preserved, while retaining the existing non-TTL round-trip coverage.Source: Coding guidelines
internal/raftengine/etcd/external_snapshot_restore.go (1)
347-392: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuevoters/learners の分割ロジックを共通化してください
confStateForSnapshotRestorePeersの分割条件はwal_store.goのsplitPeersBySuffrageと同じです。ここを共通ヘルパーに寄せると、将来の suffrage 変更時の片修正を防げます。🤖 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 `@internal/raftengine/etcd/external_snapshot_restore.go` around lines 347 - 392, confStateForSnapshotRestorePeers の voters/learners 分割を独自実装せず、wal_store.go の splitPeersBySuffrage を共通ヘルパーとして再利用してください。splitPeersBySuffrage の戻り値から raftpb.ConfState を構築し、Suffrage の判定ロジックを一箇所に集約してください。
🤖 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 `@docs/design/2026_07_19_proposed_physical_snapshot_object_offload.md`:
- Around line 13-14: Adjust the line wrapping in the design document so “PR
`#1130`” remains on the same line, preventing “#1130” from appearing at the start
of a line and being parsed as an ATX heading.
---
Nitpick comments:
In `@internal/raftengine/etcd/external_snapshot_restore.go`:
- Around line 347-392: confStateForSnapshotRestorePeers の voters/learners
分割を独自実装せず、wal_store.go の splitPeersBySuffrage
を共通ヘルパーとして再利用してください。splitPeersBySuffrage の戻り値から raftpb.ConfState を構築し、Suffrage
の判定ロジックを一箇所に集約してください。
In `@kv/snapshot_sst_ingest_test.go`:
- Around line 14-40: Test TestKVFSMSnapshotRoundTripsSSTIngestPayload with an
additional key written via PutAt using a non-zero TTL, then include it in the
SST ingest snapshot and restore flow. After Restore, assert both the
TTL/expiration metadata and value for that key are preserved, while retaining
the existing non-TTL round-trip coverage.
🪄 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: 36b263ce-b2a6-4be9-9adf-0195c7a6881f
📒 Files selected for processing (13)
docs/design/2026_06_12_proposed_scaling_roadmap.mddocs/design/2026_07_19_implemented_pebble_sst_ingest_snapshot_transfer.mddocs/design/2026_07_19_proposed_physical_snapshot_object_offload.mdinternal/raftengine/etcd/external_snapshot_restore.gointernal/raftengine/etcd/external_snapshot_restore_test.gointernal/raftengine/etcd/grpc_transport_sst_snapshot_test.gointernal/raftengine/etcd/persisted_snapshot_export.gointernal/raftengine/etcd/persisted_snapshot_export_test.gokv/snapshot_sst_ingest_test.gostore/lsm_store.gostore/snapshot_pebble.gostore/snapshot_pebble_sst.gostore/snapshot_pebble_sst_test.go
|
Codex Review: Didn't find any major issues. Bravo. 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". |
|
Updated head: Addressed the current CodeRabbit root and relevant review-body item:
The TTL suggestion refers to #1130 base files and is outside this stacked PR diff; #1130 already reached its latest-head stop condition. Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e08e3b7daf
ℹ️ 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".
|
Updated head: Resolved the current duplicate restore peer identity finding:
Caller audit:
Validation:
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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
Dependency and scope
PR #1130 owns the Pebble SST ingest snapshot stream. This pull request treats the complete FSM payload as opaque and does not parse or duplicate that format.
Because #1130 is still in review, this pull request intentionally contains only the independently reviewable export/restore substrate. Object-store publication, runtime scheduling, retention/GC, and operator restore wiring remain later milestones in the focused design.
Safety
The exporter reads only a WAL-valid persisted Raft snapshot. Token index, metadata index, footer CRC32C, and the CRC32C recomputed during streaming must agree. The source file descriptor remains open for the whole stream, so local retention cannot replace the exported inode mid-upload.
Physical restore targets an absent data directory and copies a verified regular file through the existing atomic restore preparation. The existing logical restore API and its synthetic KV header are unchanged.
Tests
go test ./internal/raftengine/etcd -count=1 -timeout=20mgo test -race ./internal/raftengine/etcd -run 'TestPreparePhysicalSnapshotRestoreAndExportOpaquePayload|TestPersistedSnapshotExport|TestOpenPersistedSnapshotExport' -count=1 -timeout=10mgo test ./... -run '^$' -count=1 -timeout=20mgolangci-lint run ./internal/raftengine/etcd/... --timeout=10mgolangci-lint, 0 issuesgit diff --checkSummary by CodeRabbit
新機能
ドキュメント
バグ修正