-
Notifications
You must be signed in to change notification settings - Fork 51
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.
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
feat!: add goleveldb
and remove pebbledb
build flag
#202
Conversation
because goleveldb won't be the default CometBFT DB in the future.
goleveldb
build flaggoleveldb
build flag
goleveldb
build flaggoleveldb
and remove pebbledb
build flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, added a few comments.
I'd just add some additional context in the PR description to include information saying there were also minor updates to CI/linting and improvements to logic in other DBs.
Thanks!
return nil | ||
} | ||
return nil | ||
return db.db.Delete(key, pebble.Sync) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the previous logic was wrong because a nil
was returned if there was an err
, but changing to the return of db.db.Delete
an error
will be returned if there's one. I think that's the expected behavior and I'm assuming there was a bug in the old logic, just want to highlight that.
@@ -160,8 +149,7 @@ func (db *PebbleDB) Compact(start, end []byte) (err error) { | |||
if end == nil && iter.Last() { | |||
end = append(end, iter.Key()...) | |||
} | |||
err = db.db.Compact(start, end, true) | |||
return | |||
return db.db.Compact(start, end, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is OK, no need to assign to err
but this can be overridden by the defer func()
if iter.Close()
errors out. But that's the expected behavior I believe.
|
||
func TestGoLevelDBBackend(t *testing.T) { | ||
name := fmt.Sprintf("test_%x", randStr(12)) | ||
db, err := NewDB(name, GoLevelDBBackend, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted that the GoLevelDBBackend
const will always be included because it's part of db.go
which doesn't have the goleveldb
flag. It's not harmful because unused const are safe in Go, but in the future if we have more build flags, we could add something like
//go:build goleveldb
// +build goleveldb
package db
const (
GoLevelDBBackend BackendType = "goleveldb"
)
Because goleveldb won't be the default CometBFT DB in the future.
Also, minor updates to CI/linting and improvements to logic in pebbledb.