raftstore: retry pending read index requests #6348

hicqu · 2019-12-27T06:49:35Z

What have you changed?

On follwer replicas pending read index requests will be retried. So that read commands won't be blocked forever in raftstore.

What is the type of the changes?

Bugfix.

How is the PR tested?

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No.

Does this PR affect `tidb-ansible`?

No.

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Any examples? (optional)

Signed-off-by: qupeng <[email protected]>

hicqu · 2019-12-27T06:51:35Z

/run-all-tests

Signed-off-by: qupeng <[email protected]>

hicqu · 2019-12-27T06:57:21Z

/run-all-tests

hicqu · 2019-12-27T07:07:53Z

/run-all-tests

src/raftstore/store/peer.rs

Signed-off-by: qupeng <[email protected]>

hicqu · 2019-12-30T11:44:34Z

/run-all-tests

Signed-off-by: qupeng <[email protected]>

hicqu · 2019-12-30T12:37:12Z

/run-all-tests

5kbpers

LGTM

Signed-off-by: qupeng <[email protected]>

…dex-retry

Signed-off-by: qupeng <[email protected]>

hicqu · 2019-12-31T07:19:24Z

/run-all-tests

hicqu · 2020-01-02T06:02:43Z

/release

sre-bot · 2020-01-02T06:20:48Z

download tikv at http://fileserver.pingcap.net/download/builds/pingcap/tikv/pr/41771096150342e77803efef04cc57665b753288/centos7/tikv-server.tar.gz

Signed-off-by: qupeng <[email protected]>

…dex-retry

Signed-off-by: qupeng <[email protected]>

BusyJay · 2020-01-09T11:19:46Z

src/raftstore/store/peer.rs

@@ -1871,6 +1871,25 @@ impl Peer {
        Ok(())
    }

+    /// `ReadIndex` requests could be lost in network, so on followers commands could queue in
+    /// `pending_reads` forever. Sending a new `ReadIndex` periodically can resolve this.
+    pub(super) fn retry_pending_reads(&mut self) {


How about put the if check here? And I think pub is enough.

BusyJay · 2020-01-09T11:24:20Z

src/raftstore/store/read_queue.rs

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


Why not pass configuration here instead? So that only one field is necessary.

So push_back will also needs the configuration, which makes it more complex.

BusyJay · 2020-01-09T11:25:44Z

src/raftstore/store/read_queue.rs

+            return false;
+        }
+
+        self.last_retried = total + self.handled_cnt - 1;


Why minus one?

So last_retried is an index, not a length.

BusyJay · 2020-01-09T11:26:02Z

src/raftstore/store/read_queue.rs

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


Should add some comment explaining the retry strategy.

Signed-off-by: qupeng <[email protected]>

…dex-retry

BusyJay · 2020-01-09T12:32:11Z

src/raftstore/store/peer.rs

@@ -191,7 +191,7 @@ pub struct Peer {

    leader_missing_time: Option<Instant>,
    leader_lease: Lease,
-    pending_reads: ReadIndexQueue,
+    pub pending_reads: ReadIndexQueue,


Unnecessary public.

BusyJay · 2020-01-09T12:32:22Z

src/raftstore/store/peer.rs

+        }
+
+        let read = self.pending_reads.back_mut().unwrap();
+        if read.read_index.is_none() {


It must be none.

BusyJay · 2020-01-09T12:37:44Z

src/raftstore/store/read_queue.rs

+        }
+
+        if self.retry_countdown == usize::MAX {
+            self.retry_countdown = cfg.raft_election_timeout_ticks.checked_sub(1).unwrap();


Election timeout tick has to be greater than 0. count down can be zero after minusing one.

Signed-off-by: qupeng <[email protected]>

…dex-retry

BusyJay · 2020-01-09T13:02:00Z

@5kbpers PTAL, I think it's completely different from what you has reviewed.

I think it's completely different from what you has reviewed.

5kbpers

LGTM

hicqu · 2020-01-10T05:12:35Z

/run-all-tests

Signed-off-by: qupeng <[email protected]>

sre-bot · 2020-01-10T05:41:01Z

cherry pick to release-3.1 failed

Signed-off-by: qupeng <[email protected]>

raftstore: retry pending read index requests (tikv#6348) (tikv#6543) Signed-off-by: qupeng <[email protected]> raft_client: limit batch size (tikv#6993) (tikv#7076) (tikv#7087) Signed-off-by: qupeng <[email protected]> backup raw kv (tikv#6308) (tikv#7051) Co-authored-by: xinhua5 <[email protected]> Co-authored-by: MyonKeminta <[email protected]> tikv_util: fix s3 writer always creating zeroes (tikv#6675) (tikv#6967) Signed-off-by: kennytm <[email protected]> Co-authored-by: Lei Zhao <[email protected]> backup: do fill in the file size of each SST file in the response (tikv#6664) (tikv#6983) Signed-off-by: kennytm <[email protected]> *: update raft to include aggressive flow control (tikv#7078) Signed-off-by: Jay Lee <[email protected]>

Signed-off-by: qupeng <[email protected]>

raftstore: retry pending read index requests

1855098

Signed-off-by: qupeng <[email protected]>

hicqu added sig/raft Component: Raft, RaftStore, etc. type/bugfix This PR fixes a bug. needs-cherry-pick-release-3.1 Type: Need cherry pick to release 3.1 labels Dec 27, 2019

hicqu requested review from BusyJay, overvenus and 5kbpers December 27, 2019 06:50

fix a typo

4a85848

Signed-off-by: qupeng <[email protected]>

5kbpers reviewed Dec 27, 2019

View reviewed changes

src/raftstore/store/peer.rs Outdated Show resolved Hide resolved

hicqu added 2 commits December 30, 2019 17:06

Merge branch 'master' into read-index-retry

fd24032

fix follower read with hibernate

d9ef3d2

Signed-off-by: qupeng <[email protected]>

address comments

41ea574

Signed-off-by: qupeng <[email protected]>

hicqu force-pushed the read-index-retry branch from e156a04 to 41ea574 Compare December 30, 2019 12:36

Merge branch 'master' into read-index-retry

280c44d

5kbpers previously approved these changes Dec 30, 2019

View reviewed changes

hicqu added 4 commits December 31, 2019 12:45

Merge branch 'master' into read-index-retry

a17f327

address comments

1edc13e

Signed-off-by: qupeng <[email protected]>

Merge branch 'read-index-retry' of github.com:hicqu/tikv into read-in…

d9b019d

…dex-retry

address comments

86f55a2

Signed-off-by: qupeng <[email protected]>

Merge branch 'master' into read-index-retry

4177109

hicqu added 2 commits January 2, 2020 14:28

fix empty check

048963b

Signed-off-by: qupeng <[email protected]>

Merge branch 'read-index-retry' of github.com:hicqu/tikv into read-in…

2d7e5cd

…dex-retry

hicqu added 2 commits January 9, 2020 19:05

fix a test

bcc86e8

Signed-off-by: qupeng <[email protected]>

Merge branch 'master' into read-index-retry

a92f006

BusyJay reviewed Jan 9, 2020

View reviewed changes

hicqu added 4 commits January 9, 2020 20:13

address comments

5cd2307

Signed-off-by: qupeng <[email protected]>

address comments

ae8afe0

Signed-off-by: qupeng <[email protected]>

Merge branch 'read-index-retry' of github.com:hicqu/tikv into read-in…

8e9bf2b

…dex-retry

Merge branch 'master' into read-index-retry

3b2f04f

BusyJay reviewed Jan 9, 2020

View reviewed changes

hicqu added 2 commits January 9, 2020 20:50

address comments

f6fb093

Signed-off-by: qupeng <[email protected]>

Merge branch 'read-index-retry' of github.com:hicqu/tikv into read-in…

7f1d98f

…dex-retry

BusyJay approved these changes Jan 9, 2020

View reviewed changes

Merge branch 'master' into read-index-retry

760c8ab

5kbpers approved these changes Jan 10, 2020

View reviewed changes

fix test case

7c63799

Signed-off-by: qupeng <[email protected]>

hicqu merged commit 76866ae into tikv:master Jan 10, 2020

hicqu deleted the read-index-retry branch January 10, 2020 05:40

hicqu added a commit to hicqu/tikv that referenced this pull request Feb 7, 2020

raftstore: retry pending read index requests (tikv#6348)

c42c449

Signed-off-by: qupeng <[email protected]>

hicqu mentioned this pull request Feb 7, 2020

raftstore: retry pending read index requests (#6348) #6543

Merged

hicqu added a commit to hicqu/tikv that referenced this pull request Feb 7, 2020

raftstore: retry pending read index requests (tikv#6348)

8ec721f

Signed-off-by: qupeng <[email protected]>

youjiali1995 pushed a commit that referenced this pull request Mar 12, 2020

raftstore: retry pending read index requests (#6348) (#6543)

915ec56

Signed-off-by: qupeng <[email protected]>

solotzg pushed a commit to pingcap/tidb-engine-ext that referenced this pull request Mar 13, 2020

raftstore: retry pending read index requests (tikv#6348) (tikv#6543)

de9681b

Signed-off-by: qupeng <[email protected]>

c1ay pushed a commit to c1ay/tikv that referenced this pull request May 9, 2020

raftstore: retry pending read index requests (tikv#6348)

f5932a3

Signed-off-by: qupeng <[email protected]>

5kbpers mentioned this pull request Dec 14, 2020

tests: add test for replica read with split isolation #9260

Merged

BusyJay mentioned this pull request Mar 1, 2022

RaftStore panicked with out of bounds access error #12046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raftstore: retry pending read index requests #6348

raftstore: retry pending read index requests #6348

hicqu commented Dec 27, 2019

hicqu commented Dec 27, 2019

hicqu commented Dec 27, 2019

hicqu commented Dec 27, 2019

hicqu commented Dec 30, 2019

hicqu commented Dec 30, 2019

5kbpers left a comment

hicqu commented Dec 31, 2019

hicqu commented Jan 2, 2020

sre-bot commented Jan 2, 2020

BusyJay Jan 9, 2020

BusyJay Jan 9, 2020

hicqu Jan 9, 2020

BusyJay Jan 9, 2020

hicqu Jan 9, 2020

BusyJay Jan 9, 2020

BusyJay Jan 9, 2020

BusyJay Jan 9, 2020

BusyJay Jan 9, 2020

BusyJay commented Jan 9, 2020

5kbpers left a comment

hicqu commented Jan 10, 2020

sre-bot commented Jan 10, 2020

raftstore: retry pending read index requests #6348

raftstore: retry pending read index requests #6348

Conversation

hicqu commented Dec 27, 2019

What have you changed?

What is the type of the changes?

How is the PR tested?

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

Does this PR affect tidb-ansible?

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Any examples? (optional)

hicqu commented Dec 27, 2019

hicqu commented Dec 27, 2019

hicqu commented Dec 27, 2019

hicqu commented Dec 30, 2019

hicqu commented Dec 30, 2019

5kbpers left a comment

Choose a reason for hiding this comment

hicqu commented Dec 31, 2019

hicqu commented Jan 2, 2020

sre-bot commented Jan 2, 2020

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

BusyJay commented Jan 9, 2020

5kbpers left a comment

Choose a reason for hiding this comment

hicqu commented Jan 10, 2020

sre-bot commented Jan 10, 2020

Does this PR affect `tidb-ansible`?