Skip to content

store: update store/store.go StoreBlock to use batches#6018

Merged
melekes merged 3 commits intotendermint:masterfrom
githubsands:consensus
Feb 9, 2021
Merged

store: update store/store.go StoreBlock to use batches#6018
melekes merged 3 commits intotendermint:masterfrom
githubsands:consensus

Conversation

@githubsands
Copy link

@githubsands githubsands commented Jan 29, 2021

  • changes store/store.Storeblock to use batches in the mean time

store/store.go Outdated
panic(err)
}
// Save the block meta data
bs.tr.Append(func(dbm.DB) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work exactly? Is the DB argument from the function arg not used? How does the transactor relate to the underlying DB?

Copy link
Author

@githubsands githubsands Jan 29, 2021

Choose a reason for hiding this comment

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

Heres the PR for the transaction, https://github.com/tendermint/tm-db/pull/147/files within your guy's tm-db repo. The DB interface gets passed into transactor at L475. It utilizes DB inherent Set and Delete functionality when Transactor's Transact method is called.

Copy link
Author

Choose a reason for hiding this comment

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

Currently this PR does not take into account dissimilarities between the DBs except vaguely (and incompletely) with
https://github.com/tendermint/tm-db/pull/147/files#diff-63ab601287b8b9c040760fe0bdd288f55b73f37cd7e4f1e519bea2bd43a18bbaR104.

Also, as of now, it only tackles the A aspect of ACID, atomicity.

Copy link
Author

@githubsands githubsands Jan 29, 2021

Choose a reason for hiding this comment

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

As you correctly noticed the passing of DB there was unnecessary. Only needed when running Transact so the Transactor's rollback func can utilize the DB's delete method. Next commit takes into account those changes.

@githubsands githubsands changed the title [WIP] store: update store/store.go to take in transaction interface in tm-db [WIP] store: update store/store.go StoreBlock to use batches Feb 3, 2021
@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #6018 (4ee6c3a) into master (ee3f34f) will decrease coverage by 0.09%.
The diff coverage is 18.18%.

@@            Coverage Diff             @@
##           master    #6018      +/-   ##
==========================================
- Coverage   60.81%   60.72%   -0.10%     
==========================================
  Files         275      275              
  Lines       25340    25345       +5     
==========================================
- Hits        15410    15390      -20     
- Misses       8323     8346      +23     
- Partials     1607     1609       +2     
Impacted Files Coverage Δ
store/store.go 56.50% <18.18%> (-0.70%) ⬇️
crypto/sr25519/pubkey.go 43.47% <0.00%> (-8.70%) ⬇️
mempool/reactor.go 65.71% <0.00%> (-2.86%) ⬇️
consensus/reactor.go 74.40% <0.00%> (-2.51%) ⬇️
privval/signer_listener_endpoint.go 77.64% <0.00%> (-2.36%) ⬇️
p2p/transport_memory.go 86.30% <0.00%> (-1.37%) ⬇️
p2p/conn/connection.go 78.23% <0.00%> (-0.56%) ⬇️
p2p/switch.go 60.18% <0.00%> (-0.47%) ⬇️
privval/signer_endpoint.go 75.75% <0.00%> (ø)
consensus/state.go 67.72% <0.00%> (+0.18%) ⬆️
... and 3 more

@melekes
Copy link
Contributor

melekes commented Feb 4, 2021

Is this still a WIP?

@githubsands githubsands changed the title [WIP] store: update store/store.go StoreBlock to use batches store: update store/store.go StoreBlock to use batches Feb 8, 2021
@githubsands
Copy link
Author

Is this still a WIP?

no longer a WIP

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

LGTM 👍

If you're up to it, you could also provide the same change (in a new PR) to the state store (see state/store.go#save)

Would we want an entry in the changelog for this @melekes?

@melekes
Copy link
Contributor

melekes commented Feb 8, 2021

Would we want an entry in the changelog for this @melekes?

yes

@githubsands
Copy link
Author

Would we want an entry in the changelog for this @melekes?

I’ll include the change log update in this PR shortly

@githubsands
Copy link
Author

Eh should you guys update the change log? You are aware of a lot more necessary context.

@githubsands
Copy link
Author

Updated with changelog.

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

Co-authored-by: Tess Rinearson <tess.rinearson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants