Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Decouple rocksdb dependency from ethcore #8320

Merged
merged 8 commits into from
Apr 9, 2018

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Apr 5, 2018

This decouples tsdb_rocksdb from ethcore, getting us one step closer towards #7865.

There're three places where ethcore was dependent on rocksdb.

  1. ClientService database. The DB opening logic is moved to Parity CLI, and Arc<KeyValueDB> is directly passed to it.
  2. Snapshot restoration. This requires opening a new DB on the fly. Thus the solution I got is to create a KeyValueDBHandler. This embeds predefined configs (if any), and exports an open function which can be used to open a DB at a given path.
  3. Convertion from config DatabaseCompactionProfile to rocksdb CompactionProfile. This is only used by Parity CLI, so it's just moved there.

For this change, we do have several additional trait objects. But given that we are already using Arc<KeyValueDB> in many places, I don't think this would be any big deal.

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. F6-refactor 📚 Code needs refactoring. M4-core ⛓ Core client code / Rust. labels Apr 5, 2018
@sorpaas sorpaas added this to the 1.11 milestone Apr 5, 2018
Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

LGTM

@tomaka
Copy link
Contributor

tomaka commented Apr 6, 2018

This is off-topic for this PR, but eventually for #7915 the code for snapshots should be refactored in order to remove the notion of Path from it (since we shouldn't depend on a filesystem within the browser). That's not a trivial change though.

@sorpaas
Copy link
Collaborator Author

sorpaas commented Apr 6, 2018

I realized that there's an indirect dep on kvdb_rocksdb through migration. That crate, however, is not used anywhere in ethcore. So I simply fixed it by removing the dep.

I double checked. And at least from the compilation logs we indeed don't have kvdb_rocksdb dependency on ethcore this time.

@rphmeier
Copy link
Contributor

rphmeier commented Apr 6, 2018

eventually for #7915 the code for snapshots should be refactored in order to remove the notion of Path from it

Yes, a SnapshotStore trait which can be provided by a crate upstream would be useful for that.

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 9, 2018
@5chdn 5chdn merged commit c039ab7 into openethereum:master Apr 9, 2018
@5chdn 5chdn added A6-revertrevert 💀 Pull request that reverts recent changes. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Apr 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-revertrevert 💀 Pull request that reverts recent changes. F6-refactor 📚 Code needs refactoring. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants