Skip to content
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.

Sign up for GitHub

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

Jump to bottom

go tendermint badger: don't scan whole keyspace #2664

Merged
merged 5 commits into from
Feb 13, 2020
Merged

go tendermint badger: don't scan whole keyspace #2664

merged 5 commits into from
Feb 13, 2020

Conversation

pro-wh
Copy link
Contributor

@pro-wh pro-wh commented Feb 11, 2020 •
edited

when we run off the end of a range iteration

but now I need to try rewriting the whole badger iterator to make sure I understand it

@pro-wh pro-wh requested a review from Yawning February 11, 2020 01:02
@codecov
Copy link

codecov bot commented Feb 11, 2020 •
edited

Codecov Report

Merging #2664 into master will decrease coverage by 0.11%.
The diff coverage is 86.04%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2664      +/-   ##
=========================================
- Coverage   63.21%   63.1%   -0.12%     
=========================================
  Files         365     365              
  Lines       34592   34577      -15     
=========================================
- Hits        21868   21820      -48     
- Misses      10014   10045      +31     
- Partials     2710    2712       +2
Impacted Files Coverage Δ
go/consensus/tendermint/db/badger/badger.go 68.09% <86.04%> (-0.29%) ⬇️
go/worker/common/host/interface.go 38.46% <0%> (-15.39%) ⬇️
go/worker/compute/executor/committee/state.go 74.07% <0%> (-11.12%) ⬇️
go/storage/mkvs/urkel/writelog/iterator.go 92.72% <0%> (-7.28%) ⬇️
go/worker/storage/service_external.go 47.19% <0%> (-6.75%) ⬇️
go/common/grpc/auth/auth.go 88.88% <0%> (-5.56%) ⬇️
go/worker/common/host/protocol/protocol.go 65.67% <0%> (-5.23%) ⬇️
go/storage/mkvs/urkel/db/api/helpers.go 64.78% <0%> (-4.23%) ⬇️
go/worker/compute/executor/committee/node.go 62.84% <0%> (-4.16%) ⬇️
go/worker/compute/txnscheduler/committee/node.go 62.02% <0%> (-2.22%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f79998e...db9a494. Read the comment docs.

kostko
kostko reviewed Feb 11, 2020
View reviewed changes
go/consensus/tendermint/db/badger/badger.go
@@ -338,22 +338,22 @@ func (it *badgerDBIterator) Next() {
panic("Next with invalid iterator")
}

for {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, why did we keep scanning before? This looks like it kept scanning until reaching the end of the database because once the key was no longer inside the domain, we just kept going until the underlying iterator was valid.

Looking at the old bolt implementation it seems like the same was done there. Where did we get those semantics from, looking at other TM-DB backends, this doesn't seem to be the case? cc @Yawning

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember, sorry.

Copy link
Contributor Author

@pro-wh pro-wh Feb 11, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key suspect is this https://github.com/oasislabs/oasis-core/blob/v20.1.2/go/consensus/tendermint/db/badger/badger.go#L302

I don't yet understand the case where we would need that, and I want to try to write an implementation that doesn't rely on that. Our tests don't cover it either. But if it's true that we need to scan into the beginning of the range, that's why we'd need this loop.

@Yawning
Copy link
Contributor

Yawning commented Feb 11, 2020

Why do you want to merge this into stable/20.1.x instead of master?

@kostko
Copy link
Member

kostko commented Feb 11, 2020

Why do you want to merge this into stable/20.1.x instead of master?

Just wanted to say the same, so this should go into master first. We can backport it later.

@pro-wh
Copy link
Contributor Author

pro-wh commented Feb 11, 2020

I'll rebase on master

@kostko kostko changed the base branch from stable/20.1.x to master February 12, 2020 08:16
@pro-wh pro-wh mentioned this pull request Feb 12, 2020
Merged
@pro-wh pro-wh requested a review from kostko February 12, 2020 18:46
@pro-wh pro-wh force-pushed the pro-wh/bugfix/badger branch 2 times, most recently from cdc4b82 to 5c5d95c Compare February 12, 2020 18:53
@pro-wh
Copy link
Contributor Author

pro-wh commented Feb 12, 2020

Ah, coverage of the changed routines is pretty good too

@pro-wh
go tendermint badger: don't scan whole keyspace
0dde7d1
@pro-wh
add changelog
5ff93f3
@pro-wh
go tendermint db: add new test
c62e953
@pro-wh
go tendermint db: restructure badger iterator
8eb3237 
- Responsibility for correct seeking is moved down to BadgerDB.
- Responsibility for caching keys and values is moved up or down.
- Bounds checking is moved to Valid.
- Methods now don't call Close internally. Remember to call it
  yourself and don't abuse Valid.
@pro-wh
add changelog
db9a494
@pro-wh pro-wh merged commit 918c7af into master Feb 13, 2020
@pro-wh pro-wh deleted the pro-wh/bugfix/badger branch February 13, 2020 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kostko kostko kostko approved these changes

@Yawning Yawning Awaiting requested review from Yawning

@peterjgilbert peterjgilbert Awaiting requested review from peterjgilbert peterjgilbert is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pro-wh @Yawning @kostko