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

Handle read index request with lease #5401

Merged
merged 8 commits into from Sep 16, 2019

Conversation

@5kbpers
Copy link
Contributor

5kbpers commented Sep 4, 2019

What have you changed?

Currently, if a replica requests a read index, then will execute the following steps:

  • replica sends a raft message MsgReadIndex to leader
  • leader receives MsgReadIndex, then sends heartbeats to check quorum
  • replica receices MsgHeartbeat, and sends MsgHeartbeatResponse
  • leader confirms a quorum, then sends MsgReadIndexResp to the replica

These steps include 2RTT, but if a leader has a valid lease, returning its committed index as a read index would be safe, then a read index request would just cost 1RTT.
This PR implements this optimization through handling MsgReadIndex before Raft::RawNode step it.

TODO:

  • benchmark

What is the type of the changes?

  • Improvement (a change which is an improvement to an existing feature)

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 added 2 commits Sep 3, 2019
Signed-off-by: 5kbpers <tangminghua@pingcap.com>
Signed-off-by: 5kbpers <tangminghua@pingcap.com>
@5kbpers 5kbpers force-pushed the 5kbpers:read-index branch from af17a1e to 43c70f5 Sep 4, 2019
5kbpers added 3 commits Sep 4, 2019
Signed-off-by: 5kbpers <tangminghua@pingcap.com>
@5kbpers 5kbpers added the C: Raft label Sep 4, 2019
@5kbpers

This comment has been minimized.

Copy link
Contributor Author

5kbpers commented Sep 4, 2019

/release

@@ -1889,7 +1905,7 @@ impl Peer {

fn read_local<T, C>(&mut self, ctx: &mut PollContext<T, C>, req: RaftCmdRequest, cb: Callback) {
ctx.raft_metrics.propose.local_read += 1;
cb.invoke_read(self.handle_read(ctx, req, false, None))
cb.invoke_read(self.handle_read(ctx, req, false, Some(self.get_store().committed_index())))

This comment has been minimized.

Copy link
@Little-Wallace

Little-Wallace Sep 5, 2019

Contributor

Why read_index should be committed_index instead of None ? Could this function read_local be called when RaftStore deal with a message which type is ReadIndex?

This comment has been minimized.

Copy link
@hicqu

hicqu Sep 5, 2019

Contributor

Seems for read_local the cmd_type will never be CmdType::ReadIndex. So I also think it's not necessary.

This comment has been minimized.

Copy link
@5kbpers

5kbpers Sep 5, 2019

Author Contributor

Currently when a leader receives a CmdType::ReadIndex and it has a valid lease, the RequestPolicy will be ReadLocal if the read_quorum field of the request is false. Then a None value read_index will cause TiKV panic.

This comment has been minimized.

Copy link
@5kbpers

5kbpers Sep 5, 2019

Author Contributor

Here assumes that we can return committed index safely when the leader gets a lease, without sending any heartbeat

@hicqu

This comment has been minimized.

Copy link
Contributor

hicqu commented Sep 5, 2019

rest LGTM

@5kbpers 5kbpers added the S: WIP label Sep 5, 2019
@5kbpers

This comment has been minimized.

Copy link
Contributor Author

5kbpers commented Sep 6, 2019

/release

@hicqu
hicqu approved these changes Sep 6, 2019
@5kbpers 5kbpers removed the S: WIP label Sep 6, 2019
Copy link
Contributor

overvenus left a comment

👍

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 16, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 16, 2019

/run-all-tests

@sre-bot sre-bot merged commit 0c1560f into tikv:master Sep 16, 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
@5kbpers

This comment has been minimized.

Copy link
Contributor Author

5kbpers commented Sep 29, 2019

/run-cherry-picker

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 29, 2019

cherry pick to release-3.1 in PR #5562

overvenus added a commit that referenced this pull request Oct 11, 2019
* handle read index with lease

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

* enable locally read index

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

* fix clippy

Signed-off-by: 5kbpers <tangminghua@pingcap.com>
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
Projects
None yet
5 participants
You can’t perform that action at this time.