raftstore: use read index to serve read request #1642

BusyJay · 2017-02-22T07:06:30Z

This pr is a clean version for #1628. Original version has too much conflict with master, so I rewrite it again.

@siddontang @hhkbp2 @zhangjinpeng1987 PTAL

BusyJay · 2017-02-23T08:18:56Z

PTAL

siddontang · 2017-02-23T14:45:00Z

Please fix the conflict.

siddontang · 2017-02-25T12:20:20Z

src/raftstore/store/peer.rs

+        });
+        self.raft_group.advance_apply(res.apply_state.get_applied_index());
+        self.mut_store().apply_state = res.apply_state.clone();
+        if has_split {


should use blank to separate the different logic.

siddontang · 2017-02-26T05:58:53Z

src/raftstore/store/peer.rs

+            term: self.term(),
+            renew_lease_time: Some(renew_lease_time),
+        };
+        let mut req = RaftCmdRequest::new();


if we send an invalid request, seem that we will log an error when apply the error, it is a little strange.

Actually, this command should be filtered by stale epoch.

So we will log an info message for it?

siddontang · 2017-02-26T05:59:18Z

src/raftstore/store/peer.rs

+        // update leader lease.
+
+        let uuid = Uuid::new_v4();
+        if self.proposals.contains(&uuid) {


seem it is a bug for the uuid lib if we have already had a same uuid.

In contrast, with version 1 and 2 UUIDs using randomly-generated node ids, hash-based version 3 and 5 UUIDs, and random version 4 UUIDs, collisions can occur even without implementation problems, albeit with a probability so small that it can normally be ignored.

https://en.wikipedia.org/wiki/Universally_unique_identifier#Collisions

BusyJay · 2017-02-27T08:10:37Z

PTAL

siddontang · 2017-02-27T08:55:21Z

src/raftstore/store/peer.rs

@@ -690,6 +723,17 @@ impl Peer {

        let mut ready = self.raft_group.ready_since(self.last_ready_idx);

+        if ready.ss.as_ref().map_or(false, |s| s.raft_state != StateRole::Leader) {


extract this to a function

siddontang · 2017-02-27T08:59:42Z

src/raftstore/store/peer.rs

@@ -779,6 +823,31 @@ impl Peer {
            }
        }

+        let mut propose_time = None;


extract to a function

siddontang · 2017-02-27T09:07:23Z

src/raftstore/store/peer.rs

+                for (req, cb) in read.cmds {
+                    self.handle_read(req, cb);
+                }
+                propose_time = Some(read.renew_lease_time);


no need to reduce pending_reads.ready_cnt here?

ready_cnt is zero if this line is reached.

but I only see setting ready_cnt to 0 in L839

siddontang · 2017-02-27T09:22:13Z

src/raftstore/store/peer.rs

+            for (req, cb) in read.cmds {
+                let uuid = util::get_uuid_from_req(&req).unwrap();
+                apply::notify_stale_req(tag, term, uuid, cb);
+            }


do we need to set ready_cnt to 0 here?

No, committed reads can still be handled.

siddontang · 2017-02-27T09:31:59Z

PTAL @hhkbp2

hhkbp2 · 2017-02-28T09:11:32Z

src/raftstore/store/peer.rs

@@ -564,10 +598,9 @@ impl Peer {
        send_to_quorum_ts + self.cfg.raft_store_max_leader_lease
    }

-    fn update_leader_lease(&mut self, ready: &Ready) {
+    fn on_role_changed(&mut self, ready: &Ready) {
        // Update leader lease when the Raft state changes.


The comments should be updated when the function changes with its name modified.

hhkbp2 · 2017-03-01T02:22:07Z

src/raftstore/store/peer.rs

-                return false;
-            }
+            // TODO: add it in queue directly.
+            return Ok(RequestPolicy::ReadIndex);


Similarly maybe it should check commit_index == current term here and return ProposeNormal since raft will drop the request directly.

No, this pr's goal is to avoid propose read request. No matter if commit_index is equal to current term or not, this is handled properly in read_index.

hhkbp2 · 2017-03-01T02:28:06Z

src/raftstore/store/peer.rs

@@ -139,6 +163,14 @@ pub struct ConsistencyState {
    pub hash: Vec<u8>,
 }

+enum RequestPolicy {


It's nice to add some comments for each policy to describe briefly what/how is a local read, read index request, consistent read, consistent write, etc., since it helps to understand and these terms lives in related comments.

hhkbp2 · 2017-03-01T07:06:02Z

src/raftstore/store/peer.rs

@@ -599,6 +640,13 @@ impl Peer {
        self.get_store().committed_index() == self.get_store().applied_index()
    }

+    #[inline]
+    pub fn ready_to_handle_read(&self) -> bool {
+        // If committed_index doesn't equal to applied_index, written apply state may be overwritten


The comments doesn't match the code, it's possible that the committed_index != applied_index while applied_index_term == self.term().

hhkbp2 · 2017-03-01T07:28:12Z

src/raftstore/store/peer.rs

+            for state in &ready.read_states {
+                let read = &self.pending_reads.reads[self.pending_reads.ready_cnt];
+                assert_eq!(state.request_ctx.as_slice(), read.uuid.as_bytes());
+                self.pending_reads.ready_cnt += 1;


It should record state.index for the read request, and check this state.index before performing read, after the log entry of this index is applied.

any update?

hhkbp2 · 2017-03-01T09:16:52Z

tests/raftstore/test_lease_read.rs


-    // Check if the leader does a consistent read and renew its lease.
+    // Check if the leader also propose an entry renew its lease.


s/renew/to renew/

hhkbp2 · 2017-03-01T09:18:34Z

tests/raftstore/test_lease_read.rs

    let state: RaftLocalState = engine.get_msg_cf(storage::CF_RAFT, &state_key).unwrap().unwrap();
    assert_eq!(state.get_last_index(), last_index + 1);

+    // wait some time to make the proposal to be applied.


s/to make/for/

hhkbp2 · 2017-03-01T09:22:40Z

tests/raftstore/test_lease_read.rs

@@ -107,21 +127,20 @@ fn test_renew_lease<T: Simulator>(cluster: &mut Cluster<T>) {
    cluster.cfg.raft_store.raft_log_gc_threshold = 100;


Those comments in this file need to be updated, since a "consistent read" has change from "write a raft log to raft quorum" to "handle a read index request by cluster with ReadOnlyOption::Safe option enabled".

hhkbp2 · 2017-03-01T09:40:28Z

Rest LGTM

siddontang · 2017-03-01T12:09:24Z

src/raftstore/store/peer.rs

+        self.raft_group.read_index(uuid.as_bytes().to_vec());
+
+        let pending_read_count = self.raft_group.raft.pending_read_count();
+        let ready_read_count = self.raft_group.raft.ready_read_count();


checking pending_read_count or ready_read_count only is enough?

No. The message may be either marked as ready immediately or pushed to pending queue.

BusyJay · 2017-03-03T06:52:14Z

Ping

hhkbp2 · 2017-03-03T08:50:50Z

LGTM

siddontang

LGTM

BusyJay · 2017-03-03T11:38:50Z

@siddontang @hhkbp2 PTAL

siddontang · 2017-03-03T11:55:05Z

LGTM

hhkbp2 · 2017-03-03T12:45:34Z

LGTM

Signed-off-by: Ray Yan <yming0221@gmail.com>

BusyJay and others added 5 commits February 21, 2017 17:47

raftstore: use read index for quorum read

b938705

propose when trasfering leader

d370e3a

Merge branch 'master' into busyjay/use-safe-read-index-2

bb6bf17

fix lease check

1dacfb3

Merge branch 'master' into busyjay/use-safe-read-index-2

1898e35

wait for apply

4958e69

siddontang reviewed Feb 25, 2017

View reviewed changes

siddontang reviewed Feb 26, 2017

View reviewed changes

Merge branch 'master' into busyjay/use-safe-read-index-2

fc5dc45

BusyJay force-pushed the busyjay/use-safe-read-index-2 branch from c8e7234 to fc5dc45 Compare February 27, 2017 08:21

siddontang reviewed Feb 27, 2017

View reviewed changes

fix test

a522fb8

hhkbp2 reviewed Feb 28, 2017

View reviewed changes

hhkbp2 reviewed Mar 1, 2017

View reviewed changes

address comment [ci skip]

6361f04

hhkbp2 reviewed Mar 1, 2017

View reviewed changes

address comment [ci skip]

12225bf

siddontang reviewed Mar 1, 2017

View reviewed changes

siddontang approved these changes Mar 3, 2017

View reviewed changes

BusyJay added 2 commits March 3, 2017 18:11

Merge branch 'master' into busyjay/use-safe-read-index-2

c239a6f

fix out of bound access

290f113

BusyJay merged commit c9fc4fc into master Mar 3, 2017

BusyJay deleted the busyjay/use-safe-read-index-2 branch March 3, 2017 12:58

overvenus mentioned this pull request Jun 5, 2023

[Dynamic Region] Refine lease read #14876

Closed

3 tasks

iosmanthus pushed a commit to iosmanthus/tikv that referenced this pull request Oct 17, 2024

cloud-worker: disable schema manager by default (tikv#1642)

8eda57d

Signed-off-by: Ray Yan <yming0221@gmail.com>

		@@ -690,6 +723,17 @@ impl Peer {

		let mut ready = self.raft_group.ready_since(self.last_ready_idx);

		if ready.ss.as_ref().map_or(false, \|s\| s.raft_state != StateRole::Leader) {


		// Check if the leader does a consistent read and renew its lease.
		// Check if the leader also propose an entry renew its lease.

		@@ -107,21 +127,20 @@ fn test_renew_lease<T: Simulator>(cluster: &mut Cluster<T>) {
		cluster.cfg.raft_store.raft_log_gc_threshold = 100;

raftstore: use read index to serve read request #1642

raftstore: use read index to serve read request #1642

Conversation

BusyJay commented Feb 22, 2017

BusyJay commented Feb 23, 2017

siddontang commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJay commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhkbp2 commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJay commented Mar 3, 2017

hhkbp2 commented Mar 3, 2017

siddontang left a comment

Choose a reason for hiding this comment

BusyJay commented Mar 3, 2017

siddontang commented Mar 3, 2017

hhkbp2 commented Mar 3, 2017