add a BadgerDB backend #115

mvdan · 2020-06-30T17:50:57Z

With multiple commits, to show the incremental changes on top of Anton's work in
early 2019.

Updates #38.

Taken from https://gist.github.com/melekes/85b8c07dd917828dc2ac6696129b73f2 as of June 2020, plus 'go mod tidy' to add the dependencies.

Mainly, many now return errors, so we can get rid of all panics. Also add the missing BackendType.

The commented out code might be useful in the future, but it's in the git history anyway.

codecov · 2020-06-30T17:52:30Z

Codecov Report

Merging #115 into master will increase coverage by 1.75%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
+ Coverage   68.86%   70.61%   +1.75%     
==========================================
  Files          26       27       +1     
  Lines        1593     1746     +153     
==========================================
+ Hits         1097     1233     +136     
- Misses        429      441      +12     
- Partials       67       72       +5

Impacted Files	Coverage Δ
db.go	`41.17% <ø> (ø)`
badger_db.go	`88.88% <88.88%> (ø)`

Impacted Files	Coverage Δ
db.go	`41.17% <ø> (ø)`
badger_db.go	`88.88% <88.88%> (ø)`

mvdan · 2020-06-30T17:53:28Z

FYI @melekes @erikgrinaker @marbar3778

mvdan · 2020-06-30T17:56:09Z

CI is failing because interfacer is complaining about an unexported function. You should disable that linter altogether - that warning isn't particularly useful :)

melekes

great work @mvdan 👍

badger_db.go

tac0turtle

could badger_db.go be broken out to match the file structure of other dbs?

mvdan · 2020-07-01T10:28:52Z

badger_db.go is under 300 lines, so I would generally not split it up. The current file structure seems a bit weird to me, with over a dozen files under 100 lines, and over 30 files total. But if you strongly prefer consistency, I can split it up.

mvdan · 2020-07-01T10:30:30Z

Two follow-up comments:

The tests here are passing, but I assume something will fail once tendermint is properly tested with this code. Can someone with more tendermint experience help?
It would be interesting to get some benchmark numbers. I can help if you can point me at a benchmark that is bottlenecked by the database, and that reflects somewhat realistic usage.

tac0turtle · 2020-07-01T10:31:40Z

badger_db.go is under 300 lines, so I would generally not split it up. The current file structure seems a bit weird to me, with over a dozen files under 100 lines, and over 30 files total. But if you strongly prefer consistency, I can split it up.

it should be fine since there will be a followup to restructuring the package.

erikgrinaker

Awesome, thanks @mvdan! Can you add a README description and changelog entry as well please?

The tests here are passing, but I assume something will fail once tendermint is properly tested with this code. Can someone with more tendermint experience help?

The Tendermint end-to-end tests pass, but IAVL fails with a panic. I believe this is caused by the lack of iter.Valid() checks, we should add a tm-db test case for this as well:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x40 pc=0x45102e6]

goroutine 174 [running]:
testing.tRunner.func1.1(0x48400e0, 0x4f26760)
	/usr/local/Cellar/go/1.14.4/libexec/src/testing/testing.go:940 +0x2f5
testing.tRunner.func1(0xc0001dd8c0)
	/usr/local/Cellar/go/1.14.4/libexec/src/testing/testing.go:943 +0x3f9
panic(0x48400e0, 0x4f26760)
	/usr/local/Cellar/go/1.14.4/libexec/src/runtime/panic.go:969 +0x166
github.com/dgraph-io/badger/v2.(*Item).Key(...)
	/Users/erik/Projects/go/pkg/mod/github.com/dgraph-io/badger/v2@v2.0.3/iterator.go:70
github.com/dgraph-io/badger/v2.(*Iterator).Item(...)
	/Users/erik/Projects/go/pkg/mod/github.com/dgraph-io/badger/v2@v2.0.3/iterator.go:504
github.com/tendermint/tm-db.(*BadgerDB).iteratorOpts(0xc000010068, 0xc0003b6a20, 0x9, 0x9, 0xc0003b6a00, 0x9, 0x9, 0x1, 0x64, 0x1, ...)
	/Users/erik/Projects/tendermint/tm-db/badger_db.go:140 +0x266
github.com/tendermint/tm-db.(*BadgerDB).ReverseIterator(0xc000010068, 0xc0003b6a00, 0x9, 0x9, 0xc0003b6a20, 0x9, 0x9, 0x401571c, 0xc00009f100, 0x8, ...)

It would be interesting to get some benchmark numbers. I can help if you can point me at a benchmark that is bottlenecked by the database, and that reflects somewhat realistic usage.

The best option would probably be Cosmos SDK simulations, which use tm-db via IAVL. I'm not very familiar with these though, as it's managed by a different team.

badger_db.go

mvdan · 2020-07-06T10:44:00Z

Can you add a README description and changelog entry as well please?

For sure.

The Tendermint end-to-end tests pass, but IAVL fails with a panic.

Thanks, I'll take a look.

The best option would probably be Cosmos SDK simulations, which use tm-db via IAVL. I'm not very familiar with these though, as it's managed by a different team.

Sounds good - I'll wait for someone from that team to give some insight, then. Benchmarking should probably come once the reviews are done.

erikgrinaker · 2020-07-06T10:49:06Z

@alexanderbez Do you have any SDK sims that would be suitable for benchmarking database backends with realistic workloads?

alexanderbez · 2020-07-06T13:13:42Z

Only thing that comes to mind is simulation benchmarking, which you can look into. Although, you should be able to get away with a pretty easy one-off benchmark, where you create a simapp with a corresponding backend, begin block, deliver txs, end block, commit, repeat.

erikgrinaker · 2020-07-06T14:03:59Z

Thanks Bez. I don't think benchmarks are necessary to merge this, especially since we don't really have a good general-purpose benchmarking suite at the moment. If you want to spend some time on it @mvdan then it's definitely welcome, but otherwise we can punt it and do some performance optimizations later if necessary.

Passing all the relevant tests.

It's been deprecated for years and for good reason. The warning it was showing in this branch wasn't useful.

mvdan · 2020-07-08T21:02:05Z

How did you run those iavl tests, beyond replacing the tm-db module with my branch? I assume you had to make some change somewhere to use the badger backend, because I don't think iavl tests against all tm-db backends.

Also, your suspicion about that missing iter.Valid call was spot on - I added a couple of iterator test cases on an empty database, and got the same panic as you did:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x81d47d]

goroutine 90 [running]:
testing.tRunner.func1.1(0x92a2c0, 0xc8cc90)
	/home/mvdan/tip/src/testing/testing.go:1057 +0x30d
testing.tRunner.func1(0xc000082480)
	/home/mvdan/tip/src/testing/testing.go:1060 +0x41a
panic(0x92a2c0, 0xc8cc90)
	/home/mvdan/tip/src/runtime/panic.go:969 +0x175
github.com/dgraph-io/badger/v2.(*Item).Key(...)
	/home/mvdan/go/pkg/mod/github.com/dgraph-io/badger/v2@v2.0.3/iterator.go:70
github.com/dgraph-io/badger/v2.(*Iterator).Item(...)
	/home/mvdan/go/pkg/mod/github.com/dgraph-io/badger/v2@v2.0.3/iterator.go:504
github.com/tendermint/tm-db.(*BadgerDB).iteratorOpts(0xc0000a00f8, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x64, 0x1, ...)
	/home/mvdan/git/tm-db/badger_db.go:140 +0x23d
github.com/tendermint/tm-db.(*BadgerDB).ReverseIterator(0xc0000a00f8, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x10, 0xc00b2343c0, 0x0, ...)
	/home/mvdan/git/tm-db/badger_db.go:163 +0x106
github.com/tendermint/tm-db.testDBIterator(0xc000082480, 0x9a78c4, 0x8)
	/home/mvdan/git/tm-db/backend_test.go:314 +0x2b77
github.com/tendermint/tm-db.TestDBIterator.func1(0xc000082480)
	/home/mvdan/git/tm-db/backend_test.go:159 +0x45
testing.tRunner(0xc000082480, 0xc00555cc20)
	/home/mvdan/tip/src/testing/testing.go:1108 +0xef
created by testing.(*T).Run
	/home/mvdan/tip/src/testing/testing.go:1159 +0x386
exit status 2

Should be all good to go, I think, unless there are other iavl panics or errors.

mvdan · 2020-07-08T21:03:18Z

Also, I've kept multiple commits, but I've also merged some changes together to not explode this into 15 commits. I think this is nicer as a series of commits than as a single commit conflating my work with Anton's.

erikgrinaker · 2020-07-09T09:43:32Z

Thanks @mvdan! IAVL tests pass - to run them, I point go.mod to a local clone and manually update TestRandomOperations to use BadgerDB.

Could you update the README and CHANGELOG as well please?

Also, I've kept multiple commits, but I've also merged some changes together to not explode this into 15 commits. I think this is nicer as a series of commits than as a single commit conflating my work with Anton's.

Thanks! We prefer to squash-merge PRs, since this makes it easier to e.g. cherry-pick or revert them.

mvdan · 2020-07-09T10:08:37Z

Could you update the README and CHANGELOG as well please?

Whoops, forgot about those. Done.

Thanks! We prefer to squash-merge PRs, since this makes it easier to e.g. cherry-pick or revert them.

Of course - I would probably go for the same :) Do you want me to squash myself then, to provide a good commit message on the squashed commit? Or are you OK with a squashed commit message that simply lists all original commit messages?

erikgrinaker · 2020-07-09T10:10:42Z

Thanks! I can squash-merge it.

mvdan added 3 commits June 29, 2020 15:12

add Anton's first badger implementation

ad0d865

Taken from https://gist.github.com/melekes/85b8c07dd917828dc2ac6696129b73f2 as of June 2020, plus 'go mod tidy' to add the dependencies.

update to the latest tm-db interface

a5af3dd

Mainly, many now return errors, so we can get rid of all panics. Also add the missing BackendType.

remove unnecessary code

3a21ca9

The commented out code might be useful in the future, but it's in the git history anyway.

mvdan requested review from erikgrinaker, melekes and tessr as code owners June 30, 2020 17:50

melekes approved these changes Jul 1, 2020

View reviewed changes

badger_db.go Outdated Show resolved Hide resolved

badger_db.go Outdated Show resolved Hide resolved

tac0turtle reviewed Jul 1, 2020

View reviewed changes

erikgrinaker self-assigned this Jul 6, 2020

erikgrinaker suggested changes Jul 6, 2020

View reviewed changes

badger_db.go Outdated Show resolved Hide resolved

badger_db.go Outdated Show resolved Hide resolved

badger_db.go Show resolved Hide resolved

mvdan added 3 commits July 8, 2020 21:58

implement the badgerdb iterator

5c75ddf

Passing all the relevant tests.

implement badger write batches

0e1ad4e

disable the interfacer linter

b21fcd5

It's been deprecated for years and for good reason. The warning it was showing in this branch wasn't useful.

add README and CHANGELOG entries

78b6318

erikgrinaker approved these changes Jul 9, 2020

View reviewed changes

erikgrinaker merged commit ae40198 into tendermint:master Jul 9, 2020

erikgrinaker mentioned this pull request Aug 12, 2020

db: add support for badgerdb tendermint/tendermint#5233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a BadgerDB backend #115

add a BadgerDB backend #115

mvdan commented Jun 30, 2020

codecov bot commented Jun 30, 2020 •

edited

Loading

mvdan commented Jun 30, 2020

mvdan commented Jun 30, 2020

melekes left a comment

tac0turtle left a comment

mvdan commented Jul 1, 2020

mvdan commented Jul 1, 2020

tac0turtle commented Jul 1, 2020

erikgrinaker left a comment •

edited

Loading

mvdan commented Jul 6, 2020

erikgrinaker commented Jul 6, 2020

alexanderbez commented Jul 6, 2020

erikgrinaker commented Jul 6, 2020

mvdan commented Jul 8, 2020

mvdan commented Jul 8, 2020

erikgrinaker commented Jul 9, 2020

mvdan commented Jul 9, 2020

erikgrinaker commented Jul 9, 2020

add a BadgerDB backend #115

add a BadgerDB backend #115

Conversation

mvdan commented Jun 30, 2020

codecov bot commented Jun 30, 2020 • edited Loading

Codecov Report

mvdan commented Jun 30, 2020

mvdan commented Jun 30, 2020

melekes left a comment

Choose a reason for hiding this comment

tac0turtle left a comment

Choose a reason for hiding this comment

mvdan commented Jul 1, 2020

mvdan commented Jul 1, 2020

tac0turtle commented Jul 1, 2020

erikgrinaker left a comment • edited Loading

Choose a reason for hiding this comment

mvdan commented Jul 6, 2020

erikgrinaker commented Jul 6, 2020

alexanderbez commented Jul 6, 2020

erikgrinaker commented Jul 6, 2020

mvdan commented Jul 8, 2020

mvdan commented Jul 8, 2020

erikgrinaker commented Jul 9, 2020

mvdan commented Jul 9, 2020

erikgrinaker commented Jul 9, 2020

codecov bot commented Jun 30, 2020 •

edited

Loading

erikgrinaker left a comment •

edited

Loading