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

add engine_traits component #5445

Merged
merged 15 commits into from Sep 25, 2019

Conversation

@5kbpers
Copy link
Contributor

5kbpers commented Sep 11, 2019

Signed-off-by: 5kbpers tangminghua@pingcap.com

What have you changed?

Separate from #5237.
This PR places the engine traits in a new component, we will migrate to it incrementally.

What is the type of the changes?

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

How is the PR tested?

CI

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

No.

Does this PR affect tidb-ansible?

No.

@5kbpers

This comment has been minimized.

Copy link
Contributor Author

5kbpers commented Sep 11, 2019

@aknuds1 Here we will add a component named "engine_traits" firstly, PTAL

@5kbpers 5kbpers force-pushed the 5kbpers:engine-traits branch 2 times, most recently from 1955d55 to a296cf8 Sep 11, 2019
Signed-off-by: 5kbpers <tangminghua@pingcap.com>
@5kbpers 5kbpers force-pushed the 5kbpers:engine-traits branch from a296cf8 to 77caf9d Sep 11, 2019
Signed-off-by: 5kbpers <tangminghua@pingcap.com>
@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 11, 2019

Thank you so much @5kbpers. This is very easy to understand. I appreciate it.

Organizationally there's one change I would like to see here, and that's to put the RocksDB implementation in a different crate, such that engine_traits does not depend on rocksdb.

What we want to end up with, for the sake of compile time, and also simply maintaining abstraction boundaries, is one crate that defines what an engine is (its traits), and for each implemented engine, another crate that implements those traits. That way the common code will build very fast, from there all the engines will build in parallel, and the implementation details of the engines can't leak into the common code or each other.

It makes sense for the rocksdb implementation to be in a crate named engine_rocksdb, so we'll ultimately have engine_traits, then engine_rocksdb, engine_sled, engine_mem etc. that implement the traits. engine_rocksdb of course already exists.

It is though nice to have a clean slate to write the rocks impls like you have here, where the rocks trait impls are not intermixed with the existing body of code. Keeps all the details of the rocks bindings from obscuring the trait implementations. So I think you should either: put the rocks impls in an entirely new crate that depends on both engine_traits and engine_rocksdb (perhaps with the aim of renaming engine_rocksdb to something else so that engine_rocksdb can just be used for the trait impls); or just make a new top-level module in engine_rocksdb, e.g. engine_traits_impls, that mirrors the layout of engine_traits and maintains that nice separation you've got here.

I'd also prefer if you split these two steps into two commits, one for defining the traits, and one four outlining the rocksdb implementation, though I admit that this single commit is perfectly comprehensible as as. (Edit: don't bother doing this)

Signed-off-by: 5kbpers <tangminghua@pingcap.com>
Copy link
Contributor

brson left a comment

Requesting some organizational changes per my previous comment.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 11, 2019

cc @aknuds1

5kbpers added 2 commits Sep 11, 2019
Signed-off-by: 5kbpers <tangminghua@pingcap.com>
Signed-off-by: 5kbpers <tangminghua@pingcap.com>
@5kbpers

This comment has been minimized.

Copy link
Contributor Author

5kbpers commented Sep 11, 2019

@brson Thanks! The dependency of rocksdb is removed, PTAL

[dependencies]
kvproto = { git = "https://github.com/pingcap/kvproto.git" }
protobuf = "2"
raft = "0.6.0-alpha"

This comment has been minimized.

Copy link
@breeswish

breeswish Sep 11, 2019

Member

I think it is not suitable for engine trait to rely on the raft.

This comment has been minimized.

Copy link
@5kbpers

5kbpers Sep 11, 2019

Author Contributor

We need to implement From trait for raft::Error(see errors.rs), or has any other place to put that?

This comment has been minimized.

Copy link
@breeswish

breeswish Sep 11, 2019

Member

@BusyJay Do you have some suggestions?

This comment has been minimized.

Copy link
@siddontang

siddontang Sep 11, 2019

Contributor

yes, it is strange to depend on raft in engine...

This comment has been minimized.

Copy link
@brson

brson Sep 11, 2019

Contributor

It looks to me like this conversion isn't needed in this PR. Maybe we could punt it out here and deal with it later when it is needed.

This comment has been minimized.

Copy link
@brson

brson Sep 11, 2019

Contributor

Hard to know a better solution when the use cases isn't here to look at.

This comment has been minimized.

Copy link
@siddontang

siddontang Sep 13, 2019

Contributor

@brson can Failure solve the problem? we may use downcast to covert an error.

Or we can use Box<Error> at first?

This comment has been minimized.

Copy link
@brson

brson Sep 19, 2019

Contributor

In cases like this Box<Error> seems just fine to me. It's pretty rare to ever inspect the concrete type of an error. And if the concrete type needs to be found it can be downcasted.

Copy link
Contributor

brson left a comment

Looks good to me modulo @breeswish's concern.

components/engine_traits/src/cf.rs Show resolved Hide resolved
components/engine_traits/src/errors.rs Show resolved Hide resolved
components/engine_traits/src/lib.rs Outdated Show resolved Hide resolved
components/engine_traits/src/engine.rs Outdated Show resolved Hide resolved
5kbpers added 2 commits Sep 11, 2019
Signed-off-by: 5kbpers <tangminghua@pingcap.com>
Signed-off-by: 5kbpers <tangminghua@pingcap.com>
Copy link
Contributor

overvenus left a comment

👍

Copy link
Contributor

siddontang left a comment

LGTM

Signed-off-by: 5kbpers <tangminghua@pingcap.com>
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 17, 2019

/run-all-tests

@aknuds1

This comment has been minimized.

Copy link
Contributor

aknuds1 commented Sep 19, 2019

@brson Do you still have unaddressed concerns wrt. this PR?

Copy link
Contributor

aknuds1 left a comment

I found some typos that should be fixed. Otherwise I have questions/suggestions.

components/engine_traits/src/errors.rs Outdated Show resolved Hide resolved
components/engine_traits/src/iterable.rs Outdated Show resolved Hide resolved
components/engine_traits/src/iterable.rs Outdated Show resolved Hide resolved
}
}

fn scan_impl<Iter: Iterator, F>(mut it: Iter, start_key: &[u8], mut f: F) -> Result<()>

This comment has been minimized.

Copy link
@aknuds1

aknuds1 Sep 19, 2019

Contributor

Maybe you should move the Iterator bound to the where block here in order to be consistent?

components/engine_traits/src/iterable.rs Outdated Show resolved Hide resolved
components/engine_traits/src/options.rs Show resolved Hide resolved
}
}

#[inline]

This comment has been minimized.

Copy link
@aknuds1

aknuds1 Sep 19, 2019

Contributor

Is the inline attribute necessary, as the Rust compiler should have heuristics to determine when to inline?

This comment has been minimized.

Copy link
@5kbpers

5kbpers Sep 19, 2019

Author Contributor

Yes it is not necessary, I will remove it

}

#[inline]
pub fn fill_cache(&mut self, v: bool) {

This comment has been minimized.

Copy link
@aknuds1

aknuds1 Sep 19, 2019

Contributor

Is there any need for this method, as opposed to setting this public attribute directly?

This comment has been minimized.

Copy link
@5kbpers

5kbpers Sep 19, 2019

Author Contributor

I prefer to keep this setter function, will remove the public attribute, thx! :)

components/engine_traits/src/peekable.rs Outdated Show resolved Hide resolved
components/engine_traits/src/peekable.rs Outdated Show resolved Hide resolved
Copy link
Contributor

brson left a comment

This looks good to me now. It looks like @5kbpers you've removed the rocks impls from this PR. I'm sorry if I sounded like I wanted that, but still this is a fine place to start.

@aknuds1 still has some points of clarification that I hope you will address, so I'm not clicking the approve button yet. I don't have opinions about the whitespace issues but the typos are worth fixing.

@brson brson dismissed their stale review Sep 19, 2019

fixed

@5kbpers 5kbpers dismissed stale reviews from siddontang and overvenus via cecc779 Sep 19, 2019
Signed-off-by: 5kbpers <tangminghua@pingcap.com>
@5kbpers 5kbpers force-pushed the 5kbpers:engine-traits branch from cecc779 to 848483d Sep 19, 2019
@5kbpers

This comment has been minimized.

Copy link
Contributor Author

5kbpers commented Sep 19, 2019

Comments addressed :), PTAL @aknuds1

Signed-off-by: 5kbpers <tangminghua@pingcap.com>
Copy link
Contributor

aknuds1 left a comment

LGTM!

@brson
brson approved these changes Sep 22, 2019
Copy link
Contributor

brson left a comment

lgtm

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 22, 2019

this is ready to land. ptal @overvenus @breeswish

@5kbpers 5kbpers requested review from breeswish, siddontang and overvenus Sep 24, 2019
@overvenus

This comment has been minimized.

Copy link
Contributor

overvenus commented Sep 25, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 25, 2019

Your auto merge job has been accepted, waiting for 5514

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 25, 2019

/run-all-tests

@sre-bot sre-bot merged commit 9535a45 into tikv:master Sep 25, 2019
6 checks passed
6 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-cop-push-down-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
Signed-off-by: 5kbpers <tangminghua@pingcap.com>
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.