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

txn: refactor commands and lock_manager #5935

Merged
merged 6 commits into from Nov 19, 2019
Merged

txn: refactor commands and lock_manager #5935

merged 6 commits into from Nov 19, 2019

Conversation

@nrc
Copy link
Contributor

nrc commented Nov 18, 2019

What have you changed?

This PR refactors storage/commands.rs. It separates Command into a Command struct and CommandKind enum to make it easier to access ctx. It removes some dead code and moves around some functions closer to where they are used. It renames PointGetCommand to BatchedGetCommand, which I hope is a bit clearer. Finally, it gets rid of storage::lock_manager, mostly by moving code into server::lock_manager, but also moving some to other more appropriate places.

I also tried chaning Command into a trait and structs rather than enums, but I don't think there was an improvement.

Each commit is self-contained.

What is the type of the changes?

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

PTAL @youjiali1995 @MyonKeminta

@nrc nrc added the C: Txn label Nov 18, 2019
@nrc nrc requested review from MyonKeminta and youjiali1995 Nov 18, 2019
Copy link
Contributor

MyonKeminta left a comment

@youjiali1995 PTAL. What's your opinion?

@@ -34,6 +35,7 @@ use self::kv::with_tls_engine;
use self::metrics::*;
use self::mvcc::{Lock, TsSet};
use self::txn::scheduler::Scheduler as TxnScheduler;
use crate::server::lock_manager::{DummyLockMgr, LockMgr};

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Nov 18, 2019

Contributor

Oh, server shouldn't be storage's dependency, I think. server combines components together, and storage may be separated to a workspace in the future.

This comment has been minimized.

Copy link
@nrc

nrc Nov 18, 2019

Author Contributor

OK, I'll shuffle stuff around a bit...

@@ -12,23 +12,25 @@ use crate::storage::types::{Key, Mutation};
pub const CMD_TAG_GC: &str = "gc";
pub const CMD_TAG_UNSAFE_DESTROY_RANGE: &str = "unsafe_destroy_range";

pub struct PointGetCommand {
/// A 'get' command which is part of a batched get operation.
pub struct BatchedGetCommand {

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Nov 18, 2019

Contributor

I think this renaming doesn't seems very important, but I'm afraid it may be misunderstood as a batch of many get commands.

This comment has been minimized.

Copy link
@nrc

nrc Nov 18, 2019

Author Contributor

I hope it's clear from the struct layout or docs that is not the case. I found PointGetCommand to be a very bad name since even from looking at the declaration it is not clear what it is.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Nov 18, 2019

Contributor

I think PointGetCommand is better... It's just a command like other commands in src/storage/commands.rs. It doesn't know how the user preprocesses it.

This comment has been minimized.

Copy link
@nrc

nrc Nov 18, 2019

Author Contributor

But what does "Point" mean? And afaict it is only used in batches, which is why it is not part of the Command enum. Maybe the name is fine but I don't understand how it is being used. In which case we can document it better.

@nrc nrc force-pushed the nrc:refactor4 branch from 89581b6 to d8269c1 Nov 18, 2019
@nrc

This comment has been minimized.

Copy link
Contributor Author

nrc commented Nov 18, 2019

@MyonKeminta I think I've addressed all your comments, PTAL.

Copy link
Contributor

MyonKeminta left a comment

Mostly LGTM, but for the renaming for PointGetCommand, I want to know other reviewers' opinions first.

components/keys/Cargo.toml Show resolved Hide resolved
@nrc nrc force-pushed the nrc:refactor4 branch from d8269c1 to 02bd3f7 Nov 18, 2019
Copy link
Contributor

youjiali1995 left a comment

LGTM

nrc added 5 commits Nov 17, 2019
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>
Signed-off-by: Nick Cameron <nrc@ncameron.org>
@nrc nrc force-pushed the nrc:refactor4 branch from 02bd3f7 to 052c7ec Nov 19, 2019
@nrc

This comment has been minimized.

Copy link
Contributor Author

nrc commented Nov 19, 2019

@youjiali1995 @MyonKeminta I've removed the renaming of PointGetCommand PTAL

Copy link
Contributor

MyonKeminta left a comment

LGTM

Copy link
Contributor

youjiali1995 left a comment

LGTM

@youjiali1995

This comment has been minimized.

Copy link
Contributor

youjiali1995 commented Nov 19, 2019

/merge

@sre-bot sre-bot added the S: CanMerge label Nov 19, 2019
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 19, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 19, 2019

@nrc merge failed.

@youjiali1995

This comment has been minimized.

Copy link
Contributor

youjiali1995 commented Nov 19, 2019

/test

@nrc

This comment has been minimized.

Copy link
Contributor Author

nrc commented Nov 19, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 19, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 19, 2019

@nrc merge failed.

@nrc

This comment has been minimized.

Copy link
Contributor Author

nrc commented Nov 19, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 19, 2019

/run-all-tests

@sre-bot sre-bot merged commit 50d27fc into tikv:master Nov 19, 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-copr-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-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
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
Projects
None yet
4 participants
You can’t perform that action at this time.