STALLED: db: add BadgerDB #58

odeke-em · 2017-09-30T23:40:02Z

Updates #30

Adds a backend for BadgerDB as requested at
#30 (comment)
and tests for it along.

BadgerDB has a few problems:
a) It doesn't have a BatchDelete, our fallback is thus Delete
because if do DeleteAsync, by the time batch.Write is invoked,
we can't be sure that the deletions have ran
b) Retrieving values from iterators requires a secondary allocation
equal to the length of the current key.
See https://godoc.org/github.com/dgraph-io/badger#KV.NewIterator
c) It seems to have arbitrary options for the size of the
log file varying between 10MB to 2GB. Currently with our
DB API, we can't pass in the required file size unless we
invoke:

db.NewBadgerDBWithOptions(&db.Options{
  ValueLogFileSize: desiredFileSize,
})

Anyways, I've arbitrarily set the default value log file size to 1GB
given that Tendermint does a whole lot of heavy lifting with
keyvalue stores <-- cite my sources

melekes · 2017-10-01T16:10:27Z

db/badger_db.go

+//  db.Set([]byte("foo"), []byte("bar"))
+//  db.SetSync([]byte("true"), []byte("tendermint"))
+//  db.Delete([]byte("true"))
+//  db.Deletesync([]byte("true"))


DeleteSync

Thanks, I'll update that.

melekes · 2017-10-01T16:14:30Z

db/badger_db.go

+func NewBadgerDB(dbName, dir string) (*BadgerDB, error) {
+	// BadgerDB doesn't expose a way for us to
+	// create  a DB with the user's supplied name.
+	if err := os.MkdirAll(dir, 0755); err != nil {


Don't you think MkdirAll belongs to NewBadgerDBWithOptions function?

I don't think so, db.NewDB uses NewBadgerDB and expects the directory making to be handled magically. Moving it out of NewBadgerDB would mean that we'd have to document separately that BadgerDB needs to have its directory made before-hand, and that would make it awkward.

melekes · 2017-10-01T16:16:16Z

db/badger_db.go

+	if err := os.MkdirAll(dir, 0755); err != nil {
+		return nil, err
+	}
+	opts := new(badger.Options)


what if we just do opts := badger.DefaultOptions?

For some reason, I initially thought badger.DefaultOptions was a pointer but somewhere I noticed not and left that in, lol. I'll update it, thank you!

melekes · 2017-10-01T16:17:25Z

db/badger_db.go

+func (b *BadgerDB) Get(key []byte) []byte {
+	valueItem := new(badger.KVItem)
+	if err := b.kv.Get(key, valueItem); err != nil {
+		// Unfortunate that Get can't return errors


melekes · 2017-10-01T16:18:52Z

db/badger_db.go

+	}
+	var valueSave []byte
+	err := valueItem.Value(func(origValue []byte) error {
+		// TODO: Decide if we should just assign valueSave to origValue


so why can't we just assign?

melekes · 2017-10-01T16:23:32Z

db/badger_db.go

+// Hesitant to do DeleteAsync because that changes the
+// expected ordering
+func (bb *badgerDBBatch) Delete(key []byte) {
+	bb.db.Delete(key)


no error handling?

The 'error handling' present with our structure doesn't return an error yet I am apprehensive about adding panics in Delete.

melekes · 2017-10-01T16:33:58Z

db/badger_db.go

+func (bb *badgerDBBatch) Write() {
+	bb.entriesMu.Lock()
+	entries := bb.entries
+	bb.entries = nil


what if somebody calls set on this batch after this? code will panic

Nope, after this, it starts a new batch. Take a look at the code in Set https://github.com/tendermint/tmlibs/pull/58/files#diff-9a61ac3e18917559e9062e23d4e9435dR237
or

ebuchman · 2017-10-03T02:36:55Z

Cool! Can we base this on Alexis' branch (#57) ? I'd love to see some benchmarks of performance vs our Go LevelDB

melekes · 2017-10-11T08:49:59Z

also, tests are failing with a missing dependency error.

melekes · 2017-10-13T14:33:46Z

Cool! Can we base this on Alexis' branch (#57) ? I'd love to see some benchmarks of performance vs our Go LevelDB

alexis' branch was merged into develop. so this branch needs to be rebased against develop

Updates #30 Adds a backend for BadgerDB as requested at #30 (comment) and tests for it along. BadgerDB has a few problems: a) It doesn't have a BatchDelete, our fallback is thus Delete because if do DeleteAsync, by the time batch.Write is invoked, we can't be sure that the deletions have ran b) Retrieving values from iterators requires a secondary allocation equal to the length of the current key. See https://godoc.org/github.com/dgraph-io/badger#KV.NewIterator c) It seems to have arbitrary options for the size of the log file varying between 10MB to 2GB. Currently with our DB API, we can't pass in the required file size unless we invoke: ```go db.NewBadgerDBWithOptions(&db.Options{ ValueLogFileSize: desiredFileSize, }) ``` Anyways, I've arbitrarily set the default value log file size to 1GB given that Tendermint does a whole lot of heavy lifting with keyvalue stores <-- cite my sources

TODO: - IteratorPrefix does not count in prefix - badgerDBIterator#Error should return err from Key() or Value()

melekes · 2017-11-04T05:37:21Z

ok. I've rebased the PR.

@odeke-em could you address some of my comments above and TODO here 2a54ceb.

Also, I would love to see benchmarks too #58 (comment)

melekes · 2017-11-04T05:39:46Z

I also have a concern that we're adding another dependency (github.com/dgraph-io/badger) to our list of deps, which is already long. Is there a way we could only require it if it's being used?

melekes · 2017-11-04T05:40:48Z

db/badger_db.go:277:22:warning: should use buf.String() instead of string(buf.Bytes()) (S1030) (megacheck)
db/badger_db.go:282:2:warning: field mu is unused (U1000) (megacheck)
db/badger_db.go:282:2:warning: unused struct field github.com/tendermint/tmlibs/db.badgerDBIterator.mu (structcheck)
db/badger_db_test.go:88::error: wrong number of args for format in Errorf call: 2 needed but 3 args (vet)
db/badger_db_test.go:186::error: wrong number of args for format in Errorf call: 2 needed but 3 args (vet)
db/example_test.go:24::error: Example_BadgerDB has malformed example suffix: BadgerDB (vet)

cloudhead · 2017-11-04T14:40:14Z

If we're going to support a bunch of backends and don't want to pull in all the deps, we can do something like:

import "tmlibs/db/base"
import "tmlibs/db/drivers/badger"
import "tmlibs/db/drivers/leveldb"

func init() {
  base.Register("badger", badger.NewDriver())
  base.Register("leveldb", leveldb.NewDriver())
}

odeke-em · 2017-11-05T00:36:35Z

@melekes I mean do we want the DB though? If we do, I don't see how we can avoid adding the dependency.

@cloudhead what do you mean by "not pull in all the deps"? By including all those imports, the respective object code has to be compiled and linked. Also I believe we already have that style of registering backends

tmlibs/db/db.go

Lines 50 to 64 in d9525c0

    
           func registerDBCreator(backend string, creator dbCreator, force bool) { 
        
           	_, ok := backends[backend] 
        
           	if !force && ok { 
        
           		return 
        
           	} 
        
           	backends[backend] = creator 
        
           } 
        
           func NewDB(name string, backend string, dir string) DB { 
        
           	db, err := backends[backend](name, dir) 
        
           	if err != nil { 
        
           		PanicSanity(Fmt("Error initializing DB: %v", err)) 
        
           	} 
        
           	return db 
        
           }

but I perhaps don't understand what the concern here is, please help me understand it.

odeke-em · 2017-11-05T00:37:49Z

@melekes thank you for the vets, I'll update them!

@melekes

Fixed the code + tests to conform to *vet warnings and also add a getter and setter for badgerDBIterator's lastErr. Thanks to @melekes for the suggestions.

odeke-em · 2017-11-05T01:18:03Z

@melekes fist bump for the vets and suggestions to make badgerDBIterator return an error, I've updated those points in commit 534cd7b

For the benchmarks, this cafe is as cold as Siberia, let me walk home, grab dinner and work on them there.

Uncovered dgraph-io/badger#308 which perhaps will require us to use the latest BadgerDB since they no longer use v0.8(which we started with) plus until that issue is resolved, our benchmarks are invalid or spuriously error out.

cloudhead · 2017-11-06T10:29:51Z

@odeke-em sorry probably wasn't clear: the code snippet I wrote would happen user-side, not in tmlibs. So the user can import additional backends and register them with tmlibs/db. By default, tmlibs/db wouldn't come with any disk-based backends.

greg-szabo · 2018-02-06T00:57:02Z

On today's (2018 feb 5th) tendermint dev meeting Bucky mentioned that according to Bucky's and Jae's research, BadgerDB is not mature enough for us to be used. Shall this PR be closed? If not, what's the priority / use case?

odeke-em · 2018-02-06T01:27:52Z

Yeah @greg-szabo in deed, I raised some problems here #58 (comment) and also in the filed issue on BadgerDB dgraph-io/badger#308, their API had drastically changed in the span of 2 weeks, but also that the max length of their keys is 1<<16 dgraph-io/badger#308 (comment).

greg-szabo · 2018-02-06T03:23:01Z

Closing this PR. Thanks for the work on it @odeke-em , even though we pass on BadgerDB, it was an important point to raise and discuss it.

odeke-em requested review from melekes and ebuchman September 30, 2017 23:40

odeke-em force-pushed the add-badger-db branch from c89dafb to db1b5a8 Compare September 30, 2017 23:44

melekes reviewed Oct 1, 2017

View reviewed changes

melekes changed the base branch from master to develop October 13, 2017 14:31

odeke-em and others added 2 commits November 4, 2017 00:13

update deps

ac4290b

melekes force-pushed the add-badger-db branch from db1b5a8 to ac4290b Compare November 4, 2017 05:19

make tests pass

2a54ceb

TODO: - IteratorPrefix does not count in prefix - badgerDBIterator#Error should return err from Key() or Value()

db/BadgerDB: fix *vet feedback from previous commits + Iter.lastErr

534cd7b

Fixed the code + tests to conform to *vet warnings and also add a getter and setter for badgerDBIterator's lastErr. Thanks to @melekes for the suggestions.

odeke-em force-pushed the add-badger-db branch from c4a9fc5 to dfe935f Compare November 6, 2017 03:09

melekes mentioned this pull request Nov 29, 2017

Add support for other dbs #30

Closed

greg-szabo mentioned this pull request Jan 30, 2018

cleveldb socket tendermint/tendermint#1162

Closed

melekes changed the title ~~db: add BadgerDB~~ STALLED: db: add BadgerDB Feb 2, 2018

greg-szabo closed this Feb 6, 2018

odeke-em deleted the add-badger-db branch March 18, 2018 04:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STALLED: db: add BadgerDB #58

STALLED: db: add BadgerDB #58

odeke-em commented Sep 30, 2017

melekes Oct 1, 2017

odeke-em Oct 1, 2017

melekes Oct 1, 2017

odeke-em Oct 1, 2017

melekes Oct 1, 2017

odeke-em Oct 1, 2017

melekes Oct 1, 2017

melekes Oct 1, 2017

melekes Oct 1, 2017

odeke-em Nov 5, 2017

melekes Oct 1, 2017

odeke-em Nov 5, 2017

ebuchman commented Oct 3, 2017

melekes commented Oct 11, 2017

melekes commented Oct 13, 2017

melekes commented Nov 4, 2017

melekes commented Nov 4, 2017

melekes commented Nov 4, 2017

cloudhead commented Nov 4, 2017

odeke-em commented Nov 5, 2017

odeke-em commented Nov 5, 2017

odeke-em commented Nov 5, 2017

cloudhead commented Nov 6, 2017

greg-szabo commented Feb 6, 2018

odeke-em commented Feb 6, 2018

greg-szabo commented Feb 6, 2018 •

edited

Loading

STALLED: db: add BadgerDB #58

STALLED: db: add BadgerDB #58

Conversation

odeke-em commented Sep 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman commented Oct 3, 2017

melekes commented Oct 11, 2017

melekes commented Oct 13, 2017

melekes commented Nov 4, 2017

melekes commented Nov 4, 2017

melekes commented Nov 4, 2017

cloudhead commented Nov 4, 2017

odeke-em commented Nov 5, 2017

odeke-em commented Nov 5, 2017

odeke-em commented Nov 5, 2017

cloudhead commented Nov 6, 2017

greg-szabo commented Feb 6, 2018

odeke-em commented Feb 6, 2018

greg-szabo commented Feb 6, 2018 • edited Loading

greg-szabo commented Feb 6, 2018 •

edited

Loading