chore(deps): upgrade pebble to v2.1.4#541
Conversation
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.
|
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 44 minutes and 55 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 ignored due to path filters (1)
📒 Files selected for processing (6)
✨ 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 upgrades the cockroachdb/pebble dependency to v2 across the codebase and updates the associated tests. Feedback includes a suggestion to remove a redundant assertion in the tests and a recommendation to re-add a persistence test to ensure StoreLog and GetLog functionality remains intact after the migration.
| "github.com/cockroachdb/pebble/v2" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // Guards the pebble v1 → v2 migration: v2 refuses to open DBs below | ||
| // FormatFlushableIngest. | ||
| func TestPebbleStore_FormatRatchetedForV2(t *testing.T) { | ||
| // Pins the on-open format so unintended downgrades below pebble v2 | ||
| // FormatMinSupported are caught early. | ||
| func TestPebbleStore_OpensAtPinnedFormat(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| s, err := NewPebbleStore(dir) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { require.NoError(t, s.Close()) }) | ||
|
|
||
| require.GreaterOrEqual(t, s.db.FormatMajorVersion(), pebble.FormatFlushableIngest) | ||
| } | ||
|
|
||
| // Existing on-disk DBs from pre-migration binaries must be ratcheted up on | ||
| // reopen — this is the migration path for clusters that already have raft.db. | ||
| func TestPebbleStore_RatchetsExistingDB(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| // Simulate an existing on-disk DB created by a pre-migration binary, | ||
| // which left FormatMajorVersion unset (== FormatMostCompatible after | ||
| // EnsureDefaults in pebble v1). | ||
| old, err := pebble.Open(dir, &pebble.Options{ | ||
| FormatMajorVersion: pebble.FormatMostCompatible, | ||
| }) | ||
| require.NoError(t, err) | ||
| require.Equal(t, pebble.FormatMostCompatible, old.FormatMajorVersion()) | ||
| require.NoError(t, old.Close()) | ||
|
|
||
| // Reopen through the production path. | ||
| s, err := NewPebbleStore(dir) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { require.NoError(t, s.Close()) }) | ||
|
|
||
| require.GreaterOrEqual(t, s.db.FormatMajorVersion(), pebble.FormatFlushableIngest) | ||
| } | ||
|
|
||
| // The format ratchet runs during DB open; committed Raft entries must survive it. | ||
| func TestPebbleStore_PreservesLogsAcrossRatchet(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| old, err := pebble.Open(dir, &pebble.Options{ | ||
| FormatMajorVersion: pebble.FormatMostCompatible, | ||
| }) | ||
| require.NoError(t, err) | ||
| pre := &PebbleStore{db: old} | ||
| require.NoError(t, pre.StoreLog(&raft.Log{Index: 1, Term: 1, Data: []byte("hello")})) | ||
| require.NoError(t, pre.StoreLog(&raft.Log{Index: 2, Term: 1, Data: []byte("world")})) | ||
| require.NoError(t, old.Close()) | ||
|
|
||
| s, err := NewPebbleStore(dir) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { require.NoError(t, s.Close()) }) | ||
|
|
||
| var got raft.Log | ||
| require.NoError(t, s.GetLog(1, &got)) | ||
| require.Equal(t, []byte("hello"), got.Data) | ||
| require.NoError(t, s.GetLog(2, &got)) | ||
| require.Equal(t, []byte("world"), got.Data) | ||
|
|
||
| last, err := s.LastIndex() | ||
| require.NoError(t, err) | ||
| require.Equal(t, uint64(2), last) | ||
| require.Equal(t, pebble.FormatVirtualSSTables, s.db.FormatMajorVersion()) | ||
| } |
There was a problem hiding this comment.
The removal of TestPebbleStore_PreservesLogsAcrossRatchet results in a loss of functional testing for the PebbleStore implementation's persistence logic. While the specific ratchet scenario from FormatMostCompatible is no longer testable in Pebble v2 (as it refuses to open versions below FormatFlushableIngest), it is still essential to verify that StoreLog and GetLog work correctly across a database close/reopen cycle with the new library version. Consider adding a simplified persistence test that uses the pinned format version to ensure that the integration between the PebbleStore wrapper and the Pebble v2 library remains correct. Additionally, ensure that any Close() calls on the database within the test explicitly handle or log errors to prevent silent resource leaks or cleanup failures.
References
- Ensure persistence logic is verified and that resource cleanup, such as Close() calls, handles errors explicitly to avoid silent failures.
- Drop the redundant GreaterOrEqual assertion; the Equal check on FormatVirtualSSTables already covers it (Gemini review). - Re-add a round-trip persistence test (TestPebbleStore_PersistsLogs AcrossReopen) that exercises StoreLog/GetLog across a close/reopen against pebble v2, replacing the dropped ratchet variant so the StoreLog/GetLog path still has end-to-end coverage (Gemini review).
|
Thanks for the review. Pushed 270c433 addressing both items: Applied:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request upgrades the cockroachdb/pebble dependency from v1 to v2 across the codebase, updating module requirements and import paths. The test suite in internal/raftstore/pebble_test.go was refactored to verify that the store opens with the FormatVirtualSSTables major version and maintains data integrity across reopens. I have no feedback to provide.
1 participant
Summary
Upgrades
github.com/cockroachdb/pebblefrom v1.1.5 to v2.1.4.Supersedes the stale Renovate PR #289, which:
go.mod/go.sumbut kept imports under the v1 module path (pebble v2 moved the module togithub.com/cockroachdb/pebble/v2).FormatMajorVersionpinning that was the entire point of the two-stage rollout.What changed
go.mod/go.sum: pingithub.com/cockroachdb/pebble/v2v2.1.4; drop v1 indirect deps that v2 no longer pulls in; add v2-era indirects.github.com/cockroachdb/pebble/v2andgithub.com/cockroachdb/pebble/v2/vfs.internal/raftstore/pebble_test.go:TestPebbleStore_RatchetsExistingDBandTestPebbleStore_PreservesLogsAcrossRatchet— both opened a DB atFormatMostCompatibleto simulate pre-migration state, but v2 unexports that constant and refuses to open belowFormatFlushableIngest. The behavior they guarded (the ratchet) was the job of chore(pebble): pin FormatMajorVersion to ratchet DBs for pebble v2 #538 and has already been validated and deployed.TestPebbleStore_OpensAtPinnedFormatand tightened it to assert equality withFormatVirtualSSTables, 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.dbat least once under the prep binary, so on-disk format is ≥FormatVirtualSSTables— comfortably above v2FormatMinSupported(FormatFlushableIngest). v2 will open existing DBs without needing any further ratchet.Test plan
go build ./...go vet ./...go test ./...(all packages pass locally, includingstore/...MVCC + snapshot tests andinternal/raftstore/...)golangci-lint run ./... --timeout=5m— 0 issuesgo mod tidy— no further changesFollow-up
PR #289 should be closed with a pointer to this PR.