-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
add boltdb for storage #3604
add boltdb for storage #3604
Conversation
require.Nil(t, err) | ||
} | ||
|
||
func BenchmarkBoltdbRandomReadsWrites(b *testing.B) { |
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.
34-95 lines are duplicate of libs/db/go_level_db_test.go:35-96
(from dupl
)
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 think that boltdb and leveldb they are both kv database. So it should be tested in the same case. Is it right?
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.
We can probably refactor the tests to benchmark them via t.Run to remove code duplication. Can happen in a follow-up PR too
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.
Thank you for your reply.
In my opinion, the benchmark cases should be the same so that we can see which database performs better .
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.
Awesome! This is a great start @CrocdileChan 👍 Added a note about upgrading existing deps and naming suggestions. We should also write more tests.
libs/db/boltdb.go
Outdated
var bucket = []byte("boltdb_bucket") | ||
|
||
func init() { | ||
registerDBCreator(BoltDBBackend, func(name string, dir string) (DB, error) { |
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.
registerDBCreator(BoltDBBackend, func(name string, dir string) (DB, error) { | |
registerDBCreator(BoltDBBackend, func(name, dir string) (DB, error) { |
libs/db/boltdb.go
Outdated
bdb.Write() | ||
} | ||
|
||
func (bdb *BoltdbBatch) Close() {} |
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.
Do you think Close should clean bdb.buffer
?
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.
OH, I got your point.
The golang has GC and the bdb.buffer
is only a *sync.map
.
It won't return memory to the operating system even though do bdb.buffer=nil
. Actually, it is the same op in go_level_db.go
So it is fine now.
@@ -0,0 +1,253 @@ | |||
package db |
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.
Not blocking this PR but shouldn't we create a package per implementation (bolt, golevel, mem, etc)?
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.
Yes to ^. We should also use conditional compilation to only include deps & code when X db is requested (// +build boltdb
).
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've requested a PR yesterday Thanks a lot :-p |
Of course! But before we do that could you please address the above comments. |
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.
Can it be merged?
Of course! But before we do that could you please address the above comments.
OK, but I don't know why and how to modify the boltdb_test.go
. I think the benchmark cases should be the same.
If you want me to modify the benchmark case, please teach me how to do it.
Thank you very much indeed.
require.Nil(t, err) | ||
} | ||
|
||
func BenchmarkBoltdbRandomReadsWrites(b *testing.B) { |
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 think that boltdb and leveldb they are both kv database. So it should be tested in the same case. Is it right?
libs/db/boltdb.go
Outdated
bdb.Write() | ||
} | ||
|
||
func (bdb *BoltdbBatch) Close() {} |
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.
OH, I got your point.
The golang has GC and the bdb.buffer
is only a *sync.map
.
It won't return memory to the operating system even though do bdb.buffer=nil
. Actually, it is the same op in go_level_db.go
So it is fine now.
require.Nil(t, err) | ||
} | ||
|
||
func BenchmarkBoltdbRandomReadsWrites(b *testing.B) { |
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.
Thank you for your reply.
In my opinion, the benchmark cases should be the same so that we can see which database performs better .
No problem. I can work on this. |
Closing in favor of #3610 |
Description: add boltdb implement for db interface. The boltdb is safe and high performence embedded KV database. It is based on B+tree and Mmap. Now it is maintained by etcd-io org. https://github.com/etcd-io/bbolt It is much better than LSM tree (levelDB) when RANDOM and SCAN read. Replaced #3604 Refs #1835 Commits: * add boltdb for storage * update boltdb in gopkg.toml * update bbolt in Gopkg.lock * dep update Gopkg.lock * add boltdb'bucket when create Boltdb * some modifies * address my own comments * fix most of the Iterator tests for boltdb * bolt does not support concurrent writes while iterating * add WARNING word * note that ReadOnly option is not supported * fixes after Ismail's review Co-Authored-By: melekes <anton.kalyaev@gmail.com> * panic if View returns error plus specify bucket in the top comment
Description: add boltdb implement for db interface. The boltdb is safe and high performence embedded KV database. It is based on B+tree and Mmap. Now it is maintained by etcd-io org. https://github.com/etcd-io/bbolt It is much better than LSM tree (levelDB) when RANDOM and SCAN read. Replaced tendermint#3604 Refs tendermint#1835 Commits: * add boltdb for storage * update boltdb in gopkg.toml * update bbolt in Gopkg.lock * dep update Gopkg.lock * add boltdb'bucket when create Boltdb * some modifies * address my own comments * fix most of the Iterator tests for boltdb * bolt does not support concurrent writes while iterating * add WARNING word * note that ReadOnly option is not supported * fixes after Ismail's review Co-Authored-By: melekes <anton.kalyaev@gmail.com> * panic if View returns error plus specify bucket in the top comment
add boltdb implement for db interface.
The boltdb is safe and high performence embedded KV database. It is based on B+tree and Mmap.
Now it is maintained by etcd-io org. https://github.com/etcd-io/bbolt
It is much better than LSM tree (levelDB) when RANDOM and SCAN read.