chore(pebble): pin FormatMajorVersion to ratchet DBs for pebble v2#538
Conversation
Prep step for bumping github.com/cockroachdb/pebble to v2 (PR #289). Pebble v2 refuses to open DBs below FormatFlushableIngest. Both DBs in this repo (internal/raftstore/pebble.go for the Raft log/stable store, and store/lsm_store.go for the MVCC FSM store) currently open with the default FormatMajorVersion, which pebble v1 treats as FormatMostCompatible — so any on-disk state produced by a current binary is unreadable by pebble v2. Pin FormatMajorVersion to FormatVirtualSSTables (the newest format pebble v1.1.5 supports, comfortably above v2 FormatMinSupported). Pebble auto ratchets the on-disk format on open, so existing databases are upgraded in place. Rollout: deploy this change on pebble v1.1.5 first and wait for every node to successfully open its DB at least once; only then merge the v2 bump in PR #289. Regression tests in internal/raftstore/pebble_test.go cover: - fresh DB opens at a v2-compatible format - existing DBs created at FormatMostCompatible are ratcheted on reopen - committed Raft log entries survive the ratchet
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPebble DB initialization is updated across two packages to pin Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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 pins the Pebble FormatMajorVersion to FormatVirtualSSTables in both the raft store and the LSM store to ensure compatibility before upgrading to Pebble v2. It also adds comprehensive tests to verify the format ratcheting and data integrity. Feedback includes a suggestion to explicitly call EnsureDefaults() for consistency and to avoid ignoring errors from Close() calls in test cleanup.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/raftstore/pebble_test.go (1)
13-44: Nit: the two format-assertion tests could be table-driven.
TestPebbleStore_FormatRatchetedForV2andTestPebbleStore_RatchetsExistingDBshare the same post-condition (FormatMajorVersion() >= FormatFlushableIngest) and differ only in the pre-existing DB state. A table-driven test with cases{"fresh", nil}and{"existing-FormatMostCompatible", &pebble.Options{FormatMajorVersion: pebble.FormatMostCompatible}}would reduce duplication.TestPebbleStore_PreservesLogsAcrossRatchetexercises a distinct behavior and is fine standalone.As per coding guidelines: "Unit tests must be co-located with packages in
*_test.gofiles; prefer table-driven test cases".internal/raftstore/pebble.go (1)
42-48: Consider centralizing the pinned-format helper.
pebbleOptions()here anddefaultPebbleOptions()instore/lsm_store.goindependently pin the sameFormatMajorVersion. When the v2 bump lands (and likely future ratchets), keeping these in sync across two packages is a foot-gun. Consider extracting a single exported helper (e.g. aninternal/pebbleoptspackage) that both sites consume, so the target format is defined once.Not blocking for this PR — safe to defer until the v2 bump if you prefer to keep this change minimal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/raftstore/pebble.go` around lines 42 - 48, The two functions pebbleOptions() and defaultPebbleOptions() both pin the same FormatMajorVersion and should share a single exported helper to avoid drift; create a small internal package (e.g. internal/pebbleopts) that exposes a function like DefaultPinnedPebbleOptions() which returns a *pebble.Options with FormatMajorVersion set (pebble.FormatVirtualSSTables), then replace the bodies of pebbleOptions() and defaultPebbleOptions() to call that new helper so both packages consume the same canonical value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/raftstore/pebble.go`:
- Around line 42-48: The two functions pebbleOptions() and
defaultPebbleOptions() both pin the same FormatMajorVersion and should share a
single exported helper to avoid drift; create a small internal package (e.g.
internal/pebbleopts) that exposes a function like DefaultPinnedPebbleOptions()
which returns a *pebble.Options with FormatMajorVersion set
(pebble.FormatVirtualSSTables), then replace the bodies of pebbleOptions() and
defaultPebbleOptions() to call that new helper so both packages consume the same
canonical value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2809f5ef-2709-41d3-8d97-e2975193c0e8
📒 Files selected for processing (3)
internal/raftstore/pebble.gointernal/raftstore/pebble_test.gostore/lsm_store.go
- Call EnsureDefaults() in raftstore pebbleOptions() to match the lsm_store site (Gemini review). - Surface errors from Close() in test cleanup instead of swallowing them (Gemini review).
|
Thanks for the reviews. Pushed a357703 addressing the actionable items: Applied (Gemini, medium priority):
Not applying (CodeRabbit nitpicks):
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Pebble database configuration to pin the FormatMajorVersion to pebble.FormatVirtualSSTables. This change ensures that existing v1-era databases are ratcheted to a version supported by Pebble v2 before the upgrade occurs. The changes are applied to both the raft store and the LSM store, and new tests have been added to verify the ratcheting behavior and data integrity. I have no feedback to provide.
Supersedes the stale Renovate PR #289. That branch only bumped go.mod and predates the format-ratchet prep (#538), so it could not be merged as-is: - It kept imports under github.com/cockroachdb/pebble, but pebble v2 moved the module to github.com/cockroachdb/pebble/v2 and go get refuses the old path. - It reverted the FormatMajorVersion pinning added in #538, which is the entire reason the migration was two-stepped. Changes here: - go.mod / go.sum bumped to cockroachdb/pebble/v2 v2.1.4. - All source + test imports updated to the /v2 module path. - internal/raftstore/pebble_test.go: dropped the two tests that simulated a pre-migration DB by opening at FormatMostCompatible, since v2 unexports that constant and rejects opens below FormatFlushableIngest anyway; kept a single assertion that a fresh NewPebbleStore opens at the pinned FormatVirtualSSTables. Rollout prerequisite satisfied: #538 has been merged and deployed to every node, so on-disk DBs are already ratcheted above FormatMinSupported (FormatFlushableIngest) and open cleanly under v2.
## Summary Upgrades `github.com/cockroachdb/pebble` from v1.1.5 to v2.1.4. Supersedes the stale Renovate PR #289, which: - Only bumped `go.mod` / `go.sum` but kept imports under the v1 module path (pebble v2 moved the module to `github.com/cockroachdb/pebble/v2`). - Predates #538 and therefore reverts the `FormatMajorVersion` pinning that was the entire point of the two-stage rollout. ## What changed - `go.mod` / `go.sum`: pin `github.com/cockroachdb/pebble/v2` v2.1.4; drop v1 indirect deps that v2 no longer pulls in; add v2-era indirects. - All Go source + test imports updated to `github.com/cockroachdb/pebble/v2` and `github.com/cockroachdb/pebble/v2/vfs`. - `internal/raftstore/pebble_test.go`: - Dropped `TestPebbleStore_RatchetsExistingDB` and `TestPebbleStore_PreservesLogsAcrossRatchet` — both opened a DB at `FormatMostCompatible` to simulate pre-migration state, but v2 unexports that constant and refuses to open below `FormatFlushableIngest`. The behavior they guarded (the ratchet) was the job of #538 and has already been validated and deployed. - Renamed the remaining test to `TestPebbleStore_OpensAtPinnedFormat` and tightened it to assert equality with `FormatVirtualSSTables`, so an unintended downgrade of the pinned version is caught. ## Migration safety Rollout prerequisite already satisfied: #538 is merged and deployed cluster-wide. Every node has opened its `raft.db` at least once under the prep binary, so on-disk format is ≥ `FormatVirtualSSTables` — comfortably above v2 `FormatMinSupported` (`FormatFlushableIngest`). v2 will open existing DBs without needing any further ratchet. ## Test plan - [x] `go build ./...` - [x] `go vet ./...` - [x] `go test ./...` (all packages pass locally, including `store/...` MVCC + snapshot tests and `internal/raftstore/...`) - [x] `golangci-lint run ./... --timeout=5m` — 0 issues - [x] `go mod tidy` — no further changes ## Follow-up PR #289 should be closed with a pointer to this PR.
1 participant
Summary
Prep step for #289 (pebble v1.1.5 → v2.1.4 bump).
Pebble v2 sets
FormatMinSupported = FormatFlushableIngestand refuses to open DBs below that. Today both pebble instances in this repo open with defaults:internal/raftstore/pebble.go— Raft log / stable store (raft.db)store/lsm_store.go— MVCC FSM storePebble v1 treats an unset
FormatMajorVersionasFormatMostCompatible, so any DB written by current binaries is unreadable by pebble v2. Merging #289 as-is would brick existing clusters on restart.This PR pins
FormatMajorVersiontopebble.FormatVirtualSSTablesin both opens. That is the newest format pebble v1.1.5 supports and is well above v2's minimum, so:FormatVirtualSSTableswas picked as an explicit, stable constant rather thanFormatNewestso the intent is auditable and doesn't drift with future pebble patch releases.Rollout order
pebble.Open.If step 2 is skipped on even one node, that node will fail to start after the v2 bump.
Test plan
go test ./internal/raftstore/... ./store/...— passesgo test ./...— passesgolangci-lint run ./internal/raftstore/... ./store/...— cleaninternal/raftstore/pebble_test.go:TestPebbleStore_FormatRatchetedForV2— fresh DB opens ≥FormatFlushableIngestTestPebbleStore_RatchetsExistingDB— reopening a DB created atFormatMostCompatibleratchets it upTestPebbleStore_PreservesLogsAcrossRatchet— committed Raft log entries survive the ratchetSummary by CodeRabbit
Improvements
Tests