raftstore-v2: implement local read for raftstore-v2 #13375

SpadeA-Tang · 2022-08-30T03:02:19Z

What is changed and how it works?

Issue Number: Ref #12842

What's Changed:

This PR involves:

Refactor LocalReader of raftstore-v1 to make some code sharable with raftstore-v2
Implement LocalReader of raftstore-v2 and add relevant tests
Replace callback with ReadCallback trait

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Unit test
Integration test

Release note

None

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot · 2022-08-30T03:02:20Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

5kbpers
tonyxuqqi

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

BusyJay · 2022-08-30T04:57:28Z

components/test_raftstore/src/server.rs

@@ -284,7 +284,7 @@ impl ServerCluster {
            }
        }

-        let local_reader = LocalReader::new(
+        let local_reader = LocalReaderCore::new(


Why use Core?

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

5kbpers · 2022-09-28T08:18:08Z

components/raftstore-v2/src/operation/mod.rs

@@ -8,3 +8,5 @@ mod ready;
 pub use command::{SimpleWriteDecoder, SimpleWriteEncoder};
 pub use life::DestroyProgress;
 pub use ready::AsyncWriter;
+
+pub(crate) use self::query::LocalReader;


If it could be used by other mods, using pub would be better.

It's supposed not to be used by other module.

In v1's code, it's called in components/server as well.

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

SpadeA-Tang · 2022-10-08T15:02:36Z

/test

tonyxuqqi · 2022-10-10T22:19:17Z

components/raftstore-v2/src/operation/query/local.rs

+        let (msg, sub) = PeerMsg::raft_query(req.clone());
+        let mut err = errorpb::Error::default();
+        match MsgRouter::send(&self.router, region_id, msg) {
+            Ok(()) => return Ok(sub.result().await),


I'm wondering if we can clone and cache sub here.
So when sub exists, we don't need to issue the MsgRouter::send again for later requests that are before sub.result().await return . Just wait on sub.result().await. And once await returns, clear the cache.
This is to dedup unnecessary renew lease requests to raftstore.
With this, it can even remove the read index amend logic in raftstore v2. Because redundant read index messages will be greatly reduced or in fact completely removed. @BusyJay What do you think?

If it is possible, I think we can propose another PR to implement it and see the improvements.

ReadIndex amend works for requests across threads.

tonyxuqqi · 2022-10-10T22:26:49Z

components/raftstore-v2/src/operation/query/local.rs

+        let region_id = req.header.get_ref().region_id;
+        TLS_LOCAL_READ_METRICS.with(|m| m.borrow_mut().renew_lease_advance.inc());
+        // Send a read query which may renew the lease
+        let (msg, sub) = PeerMsg::raft_query(req.clone());


Same as above. If we cache the sub after send successfully, it can reduce the unnecessary renew lease requests.

tonyxuqqi · 2022-10-10T22:29:20Z

components/raftstore-v2/src/operation/mod.rs

@@ -8,3 +8,5 @@ mod ready;
 pub use command::{SimpleWriteDecoder, SimpleWriteEncoder};
 pub use life::DestroyProgress;
 pub use ready::AsyncWriter;
+
+pub(crate) use self::query::LocalReader;


In v1's code, it's called in components/server as well.

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

5kbpers · 2022-10-11T08:05:10Z

/merge

ti-chi-bot · 2022-10-11T08:05:12Z

@5kbpers: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-10-11T08:05:14Z

This pull request has been accepted and is ready to merge.

Commit hash: 0ee256b

SpadeA-Tang · 2022-10-11T08:38:24Z

/test

BusyJay · 2022-10-11T10:22:25Z

Why merge this? I think it depends on #13568

SpadeA-Tang · 2022-10-11T10:26:50Z

Why merge this? I think it depends on #13568

The circumstance here is much easier. It only has snap oepration and does not have cache.

BusyJay · 2022-10-11T10:36:42Z

The circumstance here is much easier. It only has snap oepration and does not have cache.

It doesn't have cross snapshot cache, but it may has cache within the same region. And this PR also lacks the check of last_valid_ts.

BusyJay · 2022-09-28T03:53:10Z

components/raftstore-v2/src/operation/query/local.rs

+impl<'r> SnapRequestInspector<'r> {
+    fn inspect(&mut self, req: &RaftCmdRequest) -> Result<RequestPolicy> {
+        assert!(!req.has_admin_request());
+        if req.get_requests().len() != 1


In that case, maybe we don't need RaftCmdRequest at all.

BusyJay · 2022-09-28T03:53:50Z

components/raftstore-v2/src/router/imp.rs

+        self.router.send_raft_message(msg)
+    }
+
+    pub async fn get_snapshot(


Suggested change

pub async fn get_snapshot(

pub async fn snapshot(

BusyJay · 2022-09-28T03:57:57Z

components/raftstore-v2/src/operation/query/local.rs

+            return Ok(RequestPolicy::ReadIndex);
+        }
+
+        // If applied index's term is differ from current raft's term, leader transfer


Suggested change

// If applied index's term is differ from current raft's term, leader transfer

// If applied index's term is different from current raft's term, leader transfer

BusyJay · 2022-09-28T03:59:17Z

components/raftstore-v2/src/operation/query/local.rs

+        }
+    }
+
+    fn has_applied_to_current_term(&mut self) -> bool {


Better inline these functions.

BusyJay · 2022-10-11T10:26:47Z

components/raftstore-v2/src/operation/query/local.rs

+        let (msg, sub) = PeerMsg::raft_query(req.clone());
+        let mut err = errorpb::Error::default();
+        match MsgRouter::send(&self.router, region_id, msg) {
+            Ok(()) => return Ok(sub.result().await),


ReadIndex amend works for requests across threads.

SpadeA-Tang · 2022-10-11T10:44:02Z

The circumstance here is much easier. It only has snap oepration and does not have cache.

It doesn't have cross snapshot cache, but it may has cache within the same region. And this PR also lacks the check of last_valid_ts.

Checking last_valid_ts means we have to acquire the system time twice? One before getting the snapshot and one after it.

BusyJay · 2022-10-11T10:49:34Z

Checking last_valid_ts means we have to acquire the system time twice? One before getting the snapshot and one after it.

Yes, for V1. For v2, only check if latest tablet is None is sufficient.

SpadeA-Tang added 2 commits August 29, 2022 19:15

ReadResponseTrait + LocalReaderTrait

2b16eef

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

update

f326f8f

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot added do-not-merge/needs-linked-issue release-note do-not-merge/work-in-progress contribution Type: PR - From contributors size/XXL labels Aug 30, 2022

remove trait and wrap LocalReader

faa5f05

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

BusyJay reviewed Aug 30, 2022

View reviewed changes

SpadeA-Tang added 9 commits August 30, 2022 18:27

before remove readRespnoseTrait

43fcacd

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

Remove ReadResponseTrait and cargo c pass

992a4d7

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

half way for v2's snapshot

c0d4c35

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

update snapshot in v2

8648b31

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

move router from LocalReaderCore to LocalReader

16219d6

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

clippy

f7f1cd8

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

format

86af68b

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

update

1fd8ac6

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

update

c2df8d2

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot added the needs-rebase label Sep 5, 2022

SpadeA-Tang added 5 commits September 8, 2022 10:42

snapshot update

c3db075

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

add test(partial)

bde4e3d

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

refine test

9d6a39f

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

update

6f73441

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

remove unused code

611748f

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot removed the needs-rebase label Sep 13, 2022

SpadeA-Tang changed the title ~~WIP: LocalReader refactor and use ReadCallback in both v1 and v2~~ raftstore-v2: implement LocalReader for raftstore-v2 Sep 13, 2022

ti-chi-bot added do-not-merge/release-note-label-needed and removed do-not-merge/work-in-progress release-note labels Sep 13, 2022

ti-chi-bot removed the needs-rebase label Sep 27, 2022

5kbpers reviewed Sep 28, 2022

View reviewed changes

5kbpers approved these changes Sep 28, 2022

View reviewed changes

ti-chi-bot added status/LGT1 Status: PR - There is already 1 approval needs-rebase labels Sep 28, 2022

SpadeA-Tang added 2 commits October 8, 2022 18:58

get snap race condition

b5f7e88

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

merge conflict

521f976

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot removed the needs-rebase label Oct 8, 2022

SpadeA-Tang force-pushed the localreader branch from f1dc1ad to 521f976 Compare October 8, 2022 13:56

update

26bc405

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

tonyxuqqi reviewed Oct 10, 2022

View reviewed changes

tonyxuqqi approved these changes Oct 11, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Status: PR - There are already 2 approvals and removed status/LGT1 Status: PR - There is already 1 approval labels Oct 11, 2022

update

0ee256b

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot added the status/can-merge Status: Can merge to base branch label Oct 11, 2022

Merge branch 'master' into localreader

5019283

ti-chi-bot merged commit 94c8aa5 into tikv:master Oct 11, 2022

ti-chi-bot added this to the Pool milestone Oct 11, 2022

BusyJay reviewed Oct 11, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raftstore-v2: implement local read for raftstore-v2 #13375

raftstore-v2: implement local read for raftstore-v2 #13375

SpadeA-Tang commented Aug 30, 2022 •

edited

ti-chi-bot commented Aug 30, 2022 •

edited

BusyJay Aug 30, 2022

5kbpers Sep 28, 2022

SpadeA-Tang Sep 28, 2022

tonyxuqqi Oct 10, 2022

SpadeA-Tang commented Oct 8, 2022

tonyxuqqi Oct 10, 2022

SpadeA-Tang Oct 11, 2022

BusyJay Oct 11, 2022

tonyxuqqi Oct 10, 2022

tonyxuqqi Oct 10, 2022

5kbpers commented Oct 11, 2022

ti-chi-bot commented Oct 11, 2022

ti-chi-bot commented Oct 11, 2022

SpadeA-Tang commented Oct 11, 2022

BusyJay commented Oct 11, 2022

SpadeA-Tang commented Oct 11, 2022

BusyJay commented Oct 11, 2022

BusyJay Sep 28, 2022

BusyJay Sep 28, 2022

BusyJay Sep 28, 2022

BusyJay Sep 28, 2022

BusyJay Oct 11, 2022

SpadeA-Tang commented Oct 11, 2022

BusyJay commented Oct 11, 2022

	// If applied index's term is differ from current raft's term, leader transfer
	// If applied index's term is different from current raft's term, leader transfer

raftstore-v2: implement local read for raftstore-v2 #13375

raftstore-v2: implement local read for raftstore-v2 #13375

Conversation

SpadeA-Tang commented Aug 30, 2022 • edited

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot commented Aug 30, 2022 • edited

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

SpadeA-Tang commented Oct 8, 2022

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

5kbpers commented Oct 11, 2022

ti-chi-bot commented Oct 11, 2022

ti-chi-bot commented Oct 11, 2022

SpadeA-Tang commented Oct 11, 2022

BusyJay commented Oct 11, 2022

SpadeA-Tang commented Oct 11, 2022

BusyJay commented Oct 11, 2022

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

SpadeA-Tang commented Oct 11, 2022

BusyJay commented Oct 11, 2022

SpadeA-Tang commented Aug 30, 2022 •

edited

ti-chi-bot commented Aug 30, 2022 •

edited