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 |
bootjp
commented
Jul 19, 2026
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 |
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.
Uh oh!
There was an error while loading. Please reload this page.
bootjp
commented
Jul 19, 2026
Updated head: Resolved the metadata ownership finding by clearing Validation:
@codex review |
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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
35479d5 to
9012ed1Comparebootjp
commented
Jul 19, 2026
Updated stacked head: Resolved both current-head restore membership findings:
Caller audit:
Validation:
@codex review |
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
Uh oh!
There was an error while loading. Please reload this page.
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". |
bootjp
commented
Jul 19, 2026
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".
Uh oh!
There was an error while loading. Please reload this page.
bootjp
commented
Jul 19, 2026
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". |
Uh oh!
There was an error while loading. Please reload this page.
This document declares itself the canonical ownership index, so rows that call finished work unimplemented and unowned send contributors off to write a competing design. Seven rows named work that is already an ancestor of this commit: - route catalog delta watch (#1117, 6c7a66e) -> implemented owner - SST ingest snapshot transfer (#1130, 910a97e) -> implemented owner - leader-proxy circuit breaker (#1132, 56e36e9) -> implemented owner - physical snapshot offload (#1131, 764db2d) -> partial owner, which records M0/M1 implemented and still owns M2/M3 - TSO ceiling state machine (#1150, 0e85c82) -> merged, separated from the group-0 issuance work that remains open - autosplit committed-window reader (#1152, afec059) -> M3 is partially implemented, not absent - S3 offload transport and backfill (77ea547, #1126/5eaaa05d) -> the transport path is merged; the focused design names reference counting, GC readiness, and legacy migration as the remaining blockers Each cited commit was confirmed to be an ancestor of this revision, and every referenced design document was confirmed to exist. The completion rule also could not be satisfied by every row: it demanded each implemented row's focused owner be promoted to `implemented`, but the shared Pebble cache row names only PR #1082 as its canonical owner and a pull request has no status to promote. That made the roadmap permanently ineligible for promotion regardless of what shipped. Merged implementation evidence now closes such a row; an open pull request still does not. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
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
新機能
ドキュメント
バグ修正