Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(store/v2): fix the pebbledb storage implementation #21837

Merged
merged 6 commits into from
Sep 23, 2024
Merged

fix(store/v2): fix the pebbledb storage implementation #21837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
80a39b7
fix the initial version and iterator issue
cool-develope Sep 19, 2024
93ec1d3
earlist version adjust
cool-develope Sep 20, 2024
bbc498a
Merge branch 'main' into storev2/pebbledb_fix
cool-develope Sep 20, 2024
a1c929b
add test
cool-develope Sep 20, 2024
190a98c
Merge branch 'main' into storev2/pebbledb_fix
cool-develope Sep 20, 2024
b530727
Merge branch 'main' into storev2/pebbledb_fix
julienrbrt Sep 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions store/v2/storage/pebbledb/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,27 +68,27 @@ func New(dataDir string) (*Database, error) {
return nil, fmt.Errorf("failed to open PebbleDB: %w", err)
}

pruneHeight, err := getPruneHeight(db)
earliestVersion, err := getEarliestVersion(db)
if err != nil {
return nil, fmt.Errorf("failed to get prune height: %w", err)
return nil, fmt.Errorf("failed to get the earliest version: %w", err)
}

return &Database{
storage: db,
earliestVersion: pruneHeight + 1,
earliestVersion: earliestVersion,
sync: true,
}, nil
}

func NewWithDB(storage *pebble.DB, sync bool) *Database {
pruneHeight, err := getPruneHeight(storage)
earliestVersion, err := getEarliestVersion(storage)
if err != nil {
panic(fmt.Errorf("failed to get prune height: %w", err))
panic(fmt.Errorf("failed to get the earliest version: %w", err))
}

return &Database{
storage: storage,
earliestVersion: pruneHeight + 1,
earliestVersion: earliestVersion,
sync: sync,
}
}
Expand Down Expand Up @@ -362,7 +362,10 @@ func prependStoreKey(storeKey, key []byte) []byte {
return []byte(fmt.Sprintf("%s%s", storePrefix(storeKey), key))
}

func getPruneHeight(storage *pebble.DB) (uint64, error) {
// getEarliestVersion returns the earliest version set in the database.
// It is calculated by prune height + 1. If the prune height is not set, it
// returns 0.
func getEarliestVersion(storage *pebble.DB) (uint64, error) {
Comment on lines +365 to +368
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refine the documentation comment for getEarliestVersion

The documentation comment can be improved to adhere to Go's documentation conventions:

  • Start with the function name.
  • Use complete sentences.
  • Enclose variable names in backticks for clarity.

Apply this diff to enhance the comment:

-// getEarliestVersion returns the earliest version set in the database.
-// It is calculated by prune height + 1. If the prune height is not set, it
-// returns 0.
+// getEarliestVersion returns the earliest version set in the database.
+// It is calculated by adding 1 to the `pruneHeight`. If the `pruneHeight` is not set, it returns 0.
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// getEarliestVersion returns the earliest version set in the database.
// It is calculated by prune height + 1. If the prune height is not set, it
// returns 0.
func getEarliestVersion(storage *pebble.DB) (uint64, error) {
// getEarliestVersion returns the earliest version set in the database.
// It is calculated by adding 1 to the `pruneHeight`. If the `pruneHeight` is not set, it returns 0.
func getEarliestVersion(storage *pebble.DB) (uint64, error) {

bz, closer, err := storage.Get([]byte(pruneHeightKey))
if err != nil {
if errors.Is(err, pebble.ErrNotFound) {
Expand All @@ -377,7 +380,7 @@ func getPruneHeight(storage *pebble.DB) (uint64, error) {
return 0, closer.Close()
}

return binary.LittleEndian.Uint64(bz), closer.Close()
return binary.LittleEndian.Uint64(bz) + 1, closer.Close()
}

func valTombstoned(value []byte) bool {
Expand Down
6 changes: 6 additions & 0 deletions store/v2/storage/pebbledb/iterator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ func newPebbleDBIterator(src *pebble.Iterator, prefix, mvccStart, mvccEnd []byte
// so there exists at least one version of currKey SeekLT may move to.
itr.valid = itr.source.SeekLT(MVCCEncode(currKey, itr.version+1))
}

// The cursor might now be pointing at a key/value pair that is tombstoned.
// If so, we must move the cursor.
if itr.valid && itr.cursorTombstoned() {
itr.Next()
}
}
return itr
}
Expand Down
27 changes: 27 additions & 0 deletions store/v2/storage/storage_test_suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,33 @@ func (s *StorageTestSuite) TestDatabaseIterator_ForwardIterationHigher() {
s.Require().Equal(0, count)
}

func (s *StorageTestSuite) TestDatabaseIterator_WithDelete() {
db, err := s.NewDB(s.T().TempDir())
s.Require().NoError(err)
defer db.Close()

dbApplyChangeset(s.T(), db, 1, storeKey1, [][]byte{[]byte("keyA")}, [][]byte{[]byte("value001")})
dbApplyChangeset(s.T(), db, 2, storeKey1, [][]byte{[]byte("keyA")}, [][]byte{nil}) // delete

itr, err := db.Iterator(storeKey1Bytes, 1, nil, nil)
s.Require().NoError(err)

count := 0
for ; itr.Valid(); itr.Next() {
count++
}
s.Require().Equal(1, count)

itr, err = db.Iterator(storeKey1Bytes, 2, nil, nil)
s.Require().NoError(err)

count = 0
for ; itr.Valid(); itr.Next() {
count++
}
s.Require().Equal(0, count)
}

func (s *StorageTestSuite) TestDatabase_IteratorNoDomain() {
db, err := s.NewDB(s.T().TempDir())
s.Require().NoError(err)
Expand Down
Loading