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
andynog
left a comment
There was a problem hiding this comment.
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.
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.
| } | ||
| err = db.db.Compact(start, end, true) | ||
| return | ||
| return db.db.Compact(start, end, true) |
There was a problem hiding this comment.
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.
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.