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

Consolidate rocksdb imports into one module for engine abstraction #4229

Open
brson opened this Issue Feb 19, 2019 · 4 comments

Comments

Projects
None yet
3 participants
@brson
Copy link
Contributor

brson commented Feb 19, 2019

This is a single step in abstracting the engine API.

Right now rocksdb imports are spread out across the project. If there's ever to be an abstract engine API, then those imports will need to be replaced with abstract ones. And that effort will be simpler if all the rocksdb imports come from the same module.

So this issue is to remove all the imports from the rocksdb crate and replace them with imports from tikv::storage::engine, the module where much of the other engine code already lives.

As per previous comment, the steps to do this are something like:

To me, not knowing much about the engine, I would think the first step is to centralize all calls to the rocksdb crate into one place within tikv, so the entire engine API surface is known and has a single module from which to pull out an abstract interface.

I think the place those calls should be directed to is tikv::storage::engine which reexports everything needed from tikv::storage::engine::rocksdb_engine. Thus, the next steps would be:

  • Identify all paths in the tikv crate that resolve to the rocksdb crate, and modify them to resolve to tikv::storage::engine. Test with cargo check --lib in order to just build the tikv library, and not any of the binaries, tests, etc. This will create tons of errors that need to be fixed. Renaming the rocksdb crate as described further below can help with identifying where it's used.

  • Reexport all the now-unresolved types from tikv::storage::engine::rocksdb_engine. Then reexport those reexports from tikv::storage::engine, following the existing pattern of reexporting in that module. At this point cargo check --lib should succeed.

  • Verify there are no remaining calls into the rocksdb crate by renaming the crate in Cargo.toml, like rocksdb_real = { project = "rocksdb", version = "whatever" }, and removing any extern crate rocksdb statements. There should be no errors outside of the rocksdb_engine module, and those should be resolvable with a simple use rocksdb_real as rocksdb.

  • As with the tikv crate, change all the other projects, tests, benchmarks, etc to resolve into tikv instead of rocksdb. Remove the rocksdb dependencies from the projects in the components directory. Verify them in the same way as with the tikv library by renaming the rocksdb crate in Cargo.toml.

  • As a defensive measure against anyone importing from the rocksdb crate in the future, permanently rename the rocksdb crate in the Cargo.toml file as described previously, renaming it back to rocksdb in the rocksdb_engine module with a use statement, also described previously. You might even rename it to rocksdb_do_not_use to really get the point across.

Help wanted. I can mentor, or possibly @huachaohuang.

@nhultz

This comment has been minimized.

Copy link

nhultz commented Feb 27, 2019

@brson I can take a stab at this one. Been looking for a relatively straight forward task to get my feet wet with this project.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Mar 4, 2019

@nhultz Awesome. Let me know if you run into problems.

@seansu4you87

This comment has been minimized.

Copy link

seansu4you87 commented Mar 21, 2019

Hey @brson I took a rough draft at this issue in this PR. I skipped the tests/components/benches/etc because I wanted to get some feedback on what I have so far first.

Also, I just noticed that someone else already volunteered for this issue. Sorry about that 😅, I'll check more closely next time. Hopefully work hasn't been started already, if it has then feel free to forget about my PR.

@nhultz

This comment has been minimized.

Copy link

nhultz commented Mar 21, 2019

@seansu4you87 No worries! I actually got caught up and haven't had a chance to put together much for this so all yours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.