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

raftstore/apply: make it scale #4044

Merged
merged 13 commits into from Jan 12, 2019

Conversation

@BusyJay
Copy link
Contributor

commented Jan 9, 2019

This commit uses the batch/router system introduced in #3972 and #3916
to make apply scale.

Note this commit doesn't utilize the control FSM. It creates apply
delegate directly when sending a messages. That's because all messages
in apply are order-sensitive, using control FSM to create apply delegate
can introduce messages reorder. Creating a delegate is pretty
lightweight, so put it inside send method is acceptable.

Benchmark results

We use 3 TiKVs and 1 TiDBs.

default
default

BusyJay added 2 commits Jan 9, 2019
This commit uses the batch/router system introduced in #3927 and #3916
to make apply scale.

Note this commit doesn't utilize the control FSM. It creates apply
delegate directly when sending a messages. That's because all messages
in apply are order-sensitive, using control FSM to create apply delegate
can introduce messages reorder. Creating a delegate is pretty
lightweight, so put it inside send method is acceptable.

Signed-off-by: Jay Lee <busyjaylee@gmail.com>
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
@BusyJay BusyJay requested review from overvenus, hicqu and Connor1996 Jan 9, 2019
etc/config-template.toml Outdated Show resolved Hide resolved
@coocood

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

#3972 instead of #3927 in the description.

@Connor1996 Connor1996 added the C: Raft label Jan 10, 2019
src/raftstore/store/fsm/apply.rs Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
.get_uuid()
.to_vec();
resp.mut_header().set_uuid(uuid);
if !req.get_header().get_uuid().is_empty() {

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Jan 10, 2019

Member

When the uuid will be empty?

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jan 10, 2019

Author Contributor

In most case it's empty.

src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved

pub struct ControlMsg;

pub struct ControlFsm;

This comment has been minimized.

Copy link
@overvenus

overvenus Jan 10, 2019

Contributor

Can it be private?

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jan 10, 2019

Author Contributor

I don't think it can as it's exposed in BatchSystem<T, C>.

src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
}
}
if let Some(timer) = channel_timer {
let elapsed = duration_to_sec(timer.elapsed());

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Jan 10, 2019

Member

Should stop record the time before handle_apply.

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jan 10, 2019

Author Contributor

It's not efficient to do so. As either we are only recording the wait time of first apply command, or records wait time of every apply command. Generally, handle_apply is unlikely to trigger IO operation, hence record it at the end works for both performance and accuracy.

Signed-off-by: Jay Lee <busyjaylee@gmail.com>
@BusyJay BusyJay force-pushed the BusyJay:threaded-batch-apply branch from b6e1e1d to 1a03a84 Jan 10, 2019
@overvenus

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

/run-all-tests

Signed-off-by: Jay Lee <busyjaylee@gmail.com>
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
@BusyJay

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

Please use grpcio 0.4.1 instead of 0.4.0. There is some features in 0.4.1 but not in 0.4.0.

This PR doesn't touch Cargo.toml or Cargo.lock file.

Signed-off-by: Jay Lee <busyjaylee@gmail.com>
// TODO: this check is too hacky, need to be more verbose and less buggy.
let t = match self.timer.take() {
Some(t) => t,
None => return,

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Jan 11, 2019

Member

If timer is None, don't need to call the following codes? How this guaranteed?

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jan 11, 2019

Author Contributor

Timer is set everytime start handling things. There is no strong guarantee yet. It's why there is a TODO at L397.

src/raftstore/store/fsm/apply.rs Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

/release

Signed-off-by: Jay Lee <busyjaylee@gmail.com>
@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

/release

src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Show resolved Hide resolved
src/raftstore/store/fsm/apply.rs Show resolved Hide resolved
BusyJay added 2 commits Jan 11, 2019
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
Copy link
Member

left a comment

rest LGTM

Signed-off-by: Jay Lee <busyjaylee@gmail.com>
@BusyJay

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

/run-integration-tests

normal.handle_tasks(&mut self.apply_ctx, &mut self.msg_buf);
if normal.delegate.merged {
normal.delegate.destroy(&mut self.apply_ctx);
// Set it to 0 to clear all messages remained in queue.

This comment has been minimized.

Copy link
@overvenus

overvenus Jan 11, 2019

Contributor

Why can Some(0) clear remaining messages even after this fsm is stopped?

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jan 11, 2019

Author Contributor

As the trait document says, Some(0) means as long as the channel is not empty, this fsm should be kept notified.

@BusyJay BusyJay added the P: Critical label Jan 12, 2019
Copy link
Contributor

left a comment

LGTM

Copy link
Member

left a comment

LGTM

Copy link
Member

left a comment

LGTM

@BusyJay

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2019

/run-integration-tests

@Connor1996 Connor1996 merged commit 99994ae into tikv:master Jan 12, 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-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
jenkins-ci-tikv/build Jenkins job succeeded.
Details
@BusyJay BusyJay deleted the BusyJay:threaded-batch-apply branch Jan 12, 2019
@BusyJay BusyJay referenced this pull request Feb 13, 2019
8 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.