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

More engine abstraction, work toward Peekable and Iterable for Snapshot #5901

Merged
merged 51 commits into from Nov 22, 2019

Conversation

@brson
Copy link
Contributor

brson commented Nov 14, 2019

What have you changed?

This builds on #5835. The first new commit is "Add extension methods for creating RocksEngine references".

This includes a variety of work towards engine abstraction. Much of it is shaping the engine_traits abstractions in ways that will make the porting of the engine crate easier.

I was hoping to get a little further before submitting, but it's getting toward the end of the week, and the branch is getting large.

Some things this PR does:

  • Documents what I have learned about the refactoring strategy we should use in engine_traits/src/lib.rs. This will be the basis of a call for participation.

  • In engine_rocks add a Compat trait that contains a single method c(), implemented to convert from &Arc<DB> to &RocksEngine, and from &Snapshot to &RocksSnapshot, and &SyncSnapshot to &RocksSyncSnapshot etc. These are still not used, but will soon be sprinkled all over the tikv codebase as engine is merged into engine_rocks. That is why the method name is so short - to avoid syntactic noise. I have used these extensively in the branches I've discarded while producing this PR.

  • Remove the IterOptions type from engine and instead reexport it from engine_traits. This is a useful migration strategy for "plain old data" types. engine now depends on engine_traits.

  • Make several changes to engine_traits APIs to more closely match the APIs they are replacing, and for efficiency. This is again to make porting easier. Rationale for each is in the individual commit messages.

  • Migrate callers of Snapshot::db_iterator to engine_rocks. Delete db_iterator and db_iterator_cf.

  • Add a DBVector type to engine_traits etc. for data returned from database queries. Use it in the Peekable APIs. I strongly suspect using Vec for this was going to be an unacceptable deoptimization since it forces an extra memcpy on every query.

  • Introduce tikv::into_other for converting between error types that don't have mutual dependencies. Implement IntoOther<raft::Error> for engine_traits::Error and IntoOther<kvproto::Error> for engine_traits::Error. Like Compat I have used these in the discarded branches I've created while building this PR and expect them to be important in the branches that end up working.

I started out this work attempting to migrate Iterable for Snapshot callers to engine_rocks, and ended up making a bunch of these preparatory commits; then attempting to migrate Peekable for Snapshot callers and doing the same.

Right now I think Peekable is a good target for the next steps, and my next goal is to convert impl Peekable for RegionSnapshot to use the Peekable from engine_traits. After that I think it will be possible to delete Peekable for Snapshot from engine and migrate all Snapshot Peekable usage to engine_rocks.

What is the type of the changes?

Pick one of the following and delete the others:

  • Engineering (engineering change which doesn't change any feature or fix any issue)

How is the PR tested?

cargo test --all --no-run

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

no

Does this PR affect tidb-ansible?

no

Refer to a related PR or issue link (optional)

#4184

Benchmark result if necessary (optional)

Any examples? (optional)

brson and others added 30 commits Nov 4, 2019
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Eliminates duplicate code.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
The underlying engine doesn't require this, and it just makes conversion harder.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
The underlying implementation needs to consume this type. By-value is the way
the existing API works, so this is easier to migrate.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>

pub struct RocksDBVector(RawDBVector);

impl RocksDBVector {

This comment has been minimized.

Copy link
@Connor1996

Connor1996 Nov 19, 2019

Member

I'm wondering why we need to wrap RawDBVector with RocksDBVector? why not use rocksdb::DBVector directly?

This comment has been minimized.

Copy link
@brson

brson Nov 19, 2019

Author Contributor

Without wrapping it it can't implement the DBVector trait.

This comment has been minimized.

Copy link
@Connor1996

Connor1996 Nov 21, 2019

Member

but RawDBVector does implement Debug + Deref<Target = [u8]> + for<'a> PartialEq<&'a [u8]> {}

This comment has been minimized.

Copy link
@brson

brson Nov 21, 2019

Author Contributor

Yes. It could be made to automatically implement DBVector with a blanket implementation. It's not obvious to me that that is a better solution than the newtype written here. The implementation here has the advantage that it follows a pattern that is consistent with the rest of the engine_rocks crate: defining the types that implement the traits from engine_traits. Introducing a new pattern so that an out-of-crate type can implement an engine trait increases the cognitive burden of maintaining the crate.

This comment has been minimized.

Copy link
@Connor1996
brson added 6 commits Nov 19, 2019
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>
@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 19, 2019

This is updated for master and #5835, changes titan_key_only to key_only and addresses comments. It does not reorder the arguments of get_value_cf per my comments.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 19, 2019

/test

1 similar comment
@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 19, 2019

/test

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 19, 2019

Several test failures. Seems like they might be real. I'll try to reproduce them.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 19, 2019

/test

Copy link
Member

zhangjinpeng1987 left a comment

LGTM

Signed-off-by: Brian Anderson <andersrb@gmail.com>
@brson brson added the S: DNM label Nov 21, 2019
@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 21, 2019

I keep seeing inconsistent and unexpected test failures on the CI

@brson brson removed the S: DNM label Nov 21, 2019
@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 21, 2019

/test

1 similar comment
@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Nov 21, 2019

/test

Copy link
Member

zhangjinpeng1987 left a comment

LGTM

Copy link
Member

Connor1996 left a comment

LGTM

@zhangjinpeng1987 zhangjinpeng1987 merged commit ca3ba0b into tikv:master Nov 22, 2019
3 checks passed
3 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
hawkingrei added a commit to hawkingrei/tikv that referenced this pull request Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.