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

storage: simple refactoring #5822

Merged
merged 6 commits into from
Nov 8, 2019
Merged

storage: simple refactoring #5822

merged 6 commits into from
Nov 8, 2019

Conversation

nrc
Copy link
Contributor

@nrc nrc commented Nov 6, 2019

What have you changed?

These are mostly simple code motion refactorings to split up a few big files and get rid of some utils modules.

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)

PTAL @breeswish @youjiali1995 (I recommend reviewing each commit, they standalone and each is quite simple by itself)

@nrc nrc requested review from youjiali1995 and breezewish and removed request for youjiali1995 November 6, 2019 02:18
@nrc nrc added component/storage Component: Storage, Scheduler, etc. sig/transaction SIG: Transaction labels Nov 6, 2019
src/storage/mod.rs Outdated Show resolved Hide resolved
@@ -21,18 +21,22 @@ use std::io::Error as IoError;
use std::sync::{atomic, Arc};
use std::{cmp, error, u64};

use engine::{IterOption, DATA_KEY_PREFIX_LEN};
use engine::{
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw it in the import formatting:

Group imports from the same logical project together (i.e., imports from different crates in the same project).
Group imports from external crates together; do not separate std imports from other crates.

engine crate is a component in TiKV. futures is an external crate. kvproto is a part of TiKV but in a different GitHub repo. Should we group them together?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, do you think they all belong to external crates? Only crate::.. is from the same logical project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been thinking about it since I was doing this PR :-) I think I will change that advice somewhat, it is complicated.

The grouping I;ve used here is to put the imports then the re-exports and group by std, external crates (including workspace crates), then the current crate

AndreMouche
AndreMouche previously approved these changes Nov 6, 2019
Copy link
Member

@AndreMouche AndreMouche left a comment

Choose a reason for hiding this comment

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

LGTM,great job

src/storage/types.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

Copy link
Contributor

@MyonKeminta MyonKeminta left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I like it that it breaks down some very-large files.

src/storage/kv/cursor.rs Show resolved Hide resolved
Signed-off-by: Nick Cameron <nrc@ncameron.org>
Signed-off-by: Nick Cameron <nrc@ncameron.org>
Signed-off-by: Nick Cameron <nrc@ncameron.org>
Signed-off-by: Nick Cameron <nrc@ncameron.org>
@nrc
Copy link
Contributor Author

nrc commented Nov 7, 2019

@youjiali1995 @MyonKeminta rebased and comments addressed. Please take another look

MyonKeminta
MyonKeminta previously approved these changes Nov 7, 2019
Copy link
Contributor

@MyonKeminta MyonKeminta left a comment

Choose a reason for hiding this comment

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

LGTM

@MyonKeminta
Copy link
Contributor

MyonKeminta commented Nov 7, 2019

Oh, something wrong in CI. Please fix the CI.

Signed-off-by: Nick Cameron <nrc@ncameron.org>
Copy link
Member

@AndreMouche AndreMouche left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

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

LGTM

@youjiali1995 youjiali1995 added the status/can-merge Indicates a PR has been approved by a committer. label Nov 8, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 8, 2019

/run-all-tests

@sre-bot sre-bot merged commit 1999eaf into tikv:master Nov 8, 2019
hawkingrei pushed a commit to hawkingrei/tikv that referenced this pull request Dec 1, 2019
Signed-off-by: Nick Cameron <nrc@ncameron.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/storage Component: Storage, Scheduler, etc. sig/transaction SIG: Transaction status/can-merge Indicates a PR has been approved by a committer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants