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.

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

support rocksdb v7 #1285

Closed
wants to merge 3 commits into from
Closed

support rocksdb v7 #1285

wants to merge 3 commits into from

Conversation

faddat
Copy link
Member

@faddat faddat commented Apr 18, 2022

Works towards: #695

Description

This PR migrates us away from gorocksdb wrapper library, which is no longer maintained, and to grocksdb, which is. Along the way we get support for rocksdb v7:

https://github.com/facebook/rocksdb/releases/tag/v7.0.1


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2022

Codecov Report

Merging #1285 (9538057) into main (b6d342a) will decrease coverage by 0.75%.
The diff coverage is 9.32%.

@@            Coverage Diff             @@
##             main    #1285      +/-   ##
==========================================
- Coverage   20.90%   20.14%   -0.76%     
==========================================
  Files         196      203       +7     
  Lines       25425    26824    +1399     
==========================================
+ Hits         5316     5405      +89     
- Misses      19118    20412    +1294     
- Partials      991     1007      +16     
Impacted Files Coverage Δ
x/claim/abci.go 0.00% <ø> (ø)
x/claim/client/cli/query.go 34.45% <ø> (ø)
x/claim/client/cli/tx.go 0.00% <ø> (ø)
x/claim/handler.go 0.00% <ø> (ø)
x/claim/keeper/claim.go 64.33% <ø> (ø)
x/claim/keeper/grpc_query.go 2.50% <ø> (ø)
x/claim/keeper/hooks.go 28.00% <ø> (ø)
x/claim/keeper/keeper.go 87.50% <ø> (ø)
x/claim/keeper/params.go 81.81% <ø> (ø)
x/claim/module.go 58.06% <ø> (ø)
... and 106 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 ee2fa7c...9538057. Read the comment docs.

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Shouldn't support for this version of RocksDB come from the (our) SDK dependency?

@faddat
Copy link
Member Author

faddat commented Apr 18, 2022

Oh, do you mean github.com/cosmos/gorocksdb ?

If you mean that, the answer is no. gorocksdb doesn't support the latest rocksdb, and isn't maintained.

If you mean this: tendermint/tm-db#229

the answer is no, it hasn't been merged

There's also badgerdb v3 support in this one:

tendermint/tm-db#232

Additional context here:

cosmos/gorocksdb#6 (comment)

@ValarDragon
Copy link
Member

Have you checked if we can upgrade an existing rocksDB database to this new import, and if things still work? If you've tested that and it works, then imo its good to merge!

If that doesn't work, we should call this db something else in tm-db / makefile OR communicate to all validators on the old RocksDB to switch in the interrim. (I would prefer not doing the latter option, or at minimum only considering it after state sync is up)

@faddat
Copy link
Member Author

faddat commented Apr 25, 2022

@ValarDragon

  • I think it is compatible with v6
  • I think that we were the only ones to use rocksdb as a validator, and tbh, it did not go well (yet). The biggest issue was state sync, and that broke (for rocks specifically) when we added wasm to osmosis

This is kinda too many "I thinks" for me, I'd like to suggest that we merge this when this is merged:

tendermint/tm-db#236

That way, our tm-db continues to match the one upstream (our iavl currently relies on that repo as well)

I've converted this to a draft till then & will update when upstream does

@faddat faddat marked this pull request as draft April 25, 2022 20:10
@p0mvn
Copy link
Member

p0mvn commented May 5, 2022

Hi @faddat . I'm reviewing open PRs. Are we still blocked on the PR in tm-db?

@faddat
Copy link
Member Author

faddat commented May 7, 2022

Yes. We are still blocked on that, here’s a tracker:

tendermint/tm-db#248

@faddat
Copy link
Member Author

faddat commented May 7, 2022

@marbar3778 I would like to try to resolve this.

@faddat
Copy link
Member Author

faddat commented May 18, 2022

Regarding this one I intend to bring up the core issues at play on Friday's SDK architecture review call.

I'd very much like to get it in.

@ValarDragon
Copy link
Member

Adding the blocked label for now!

@ValarDragon
Copy link
Member

ValarDragon commented Jul 6, 2022

Closing since we're not actively working on this rn / its gone stale. (trying to get the PR list down a bit)

Lets re-open when we can allocate attention again?

Seems like this is actually super close to being in a state we can use? We basically needed to:

  • Update our TM-DB version
  • See what happens if I had a rocks DB v6 version, and we switched to this for v7. (Did things break?)
  • See what happens running a rocksDB instance on mainnet / it still works

This can all be tested against v10.x as well, so we can test out there before it gets into another release

@ValarDragon ValarDragon closed this Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants