raftstore: Wait ticks for hibernated peer when doing force leader by Connor1996 · Pull Request #12364 · tikv/tikv

Connor1996 · 2022-04-15T01:48:27Z

What is changed and how it works?

Issue Number: Ref #10483

What's Changed:

Force leader is rejected on a peer who is already a leader. For the hibernated leader,
it doesn't step down to follower when quorum is missing due to not ticking election. 
So wait ticks in force leader process for hibernated peers to make sure election ticking
is performed.

Check List

Tests

Unit test

Release note

None

Signed-off-by: Connor1996 <zbk602423539@gmail.com>

ti-chi-bot · 2022-04-15T01:48:28Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

BusyJay
tabokie

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.

Details

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

Connor1996 · 2022-04-15T01:52:36Z

PTAL @v01dstar

Signed-off-by: Connor1996 <zbk602423539@gmail.com>

v01dstar · 2022-04-15T06:06:41Z

components/raftstore/src/store/fsm/peer.rs

+            );
+            if *ticks == 0 {
+                let s = mem::take(failed_stores);
+                self.on_enter_pre_force_leader(s);


Instead of calling back on on_enter_pre_force_leader() maybe it looks cleaner if we separate the on_enter_pre_force_leader() into several functions like

SignificantMsg::EnterForceLeaderState => { self.on_enter_force_leader(); } fn on_enter_force_leader() { if state.is_some() { ... } if is_leader || （promotable && has_leader）{ self.leader_absence_check() } else { self.voter_liveness_check() } } fn check_force_leader() { if leader_absence_check_passes { self.voter_liveness_check() } if voter_liveness_check_passes { ... } }

After waiting ticks, we need to re-check all the conditions in on_enter_pre_force_leader. It may still be the leader due to it doesn't lose the quorum. Reusing is straightforward.

That's also something we can optimize here, instead of waiting for certain ticks, and then check the leader absence in the on_enter_pre_force_leader(), maybe we can proactively check whether the leader is absent or not in each tick at check_force_leader()?

Can't. For leader, CheckQuorum needs to wait for a round to see if peers are recent active; for follower, only after an election timeout, it can make sure the leader lease is expired. Anyway, both of them need to wait for some ticks to get the correct information.

Signed-off-by: Connor1996 <zbk602423539@gmail.com>

…ibernate

components/raftstore/src/store/fsm/peer.rs

Signed-off-by: Connor1996 <zbk602423539@gmail.com>

components/raftstore/src/store/fsm/peer.rs

v01dstar · 2022-04-19T01:58:36Z

tests/integrations/raftstore/test_unsafe_recovery.rs

It would be better if we assert the target region is hibernated here. Something like assert_eq!(is_hibernated(region.get_id()), ture);

can't, it's not exported

Signed-off-by: Connor1996 <zbk602423539@gmail.com>

ti-chi-bot · 2022-04-20T03:39:51Z

@v01dstar: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

Details

In response to this:

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.

Connor1996 · 2022-04-20T03:58:24Z

/merge

ti-chi-bot · 2022-04-20T03:58:25Z

@Connor1996: 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.

Details

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-04-20T03:58:27Z

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

Details

Commit hash: 94449b7

ti-chi-bot · 2022-04-20T10:30:17Z

@Connor1996: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

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.

Details

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.

…kv#12364) ref tikv#10483 Force leader is rejected on a peer who is already a leader. For the hibernated leader, it doesn't step down to follower when quorum is missing due to not ticking election. So wait ticks in force leader process for hibernated peers to make sure election ticking is performed. Signed-off-by: Connor1996 <zbk602423539@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

* raftstore: Introduce force leader state (tikv#11932) close tikv#6107, ref tikv#10483 Signed-off-by: Connor1996 <zbk602423539@gmail.com> * raftstore: Wait ticks for hibernated peer when doing force leader (tikv#12364) ref tikv#10483 Force leader is rejected on a peer who is already a leader. For the hibernated leader, it doesn't step down to follower when quorum is missing due to not ticking election. So wait ticks in force leader process for hibernated peers to make sure election ticking is performed. Signed-off-by: Connor1996 <zbk602423539@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> * raftstore: Make unsafe recovery wait apply cover snapshot apply cases ref tikv#10483 (tikv#12308) ref tikv#10483 Signed-off-by: v01dstar <yang.zhang@pingcap.com> * raftstore: Execute recovery plan via raft (tikv#12022) Signed-off-by: Connor1996 <zbk602423539@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Co-authored-by: Yang Zhang <yang.zhang@pingcap.com>

add test

518f5ed

Signed-off-by: Connor1996 <zbk602423539@gmail.com>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 15, 2022

Connor1996 requested a review from BusyJay April 15, 2022 01:48

Connor1996 mentioned this pull request Apr 15, 2022

Support online recovery for Raft majority failure #10483

Closed

44 tasks

ref#10483 wait ticks for hibernated peer when doing force leader

5ab3918

Signed-off-by: Connor1996 <zbk602423539@gmail.com>

Connor1996 force-pushed the force-leader-on-hibernate branch from 23a9a75 to 5ab3918 Compare April 15, 2022 02:28

v01dstar reviewed Apr 15, 2022

View reviewed changes

Connor1996 added 2 commits April 15, 2022 14:20

clean

a41677f

Signed-off-by: Connor1996 <zbk602423539@gmail.com>

Merge remote-tracking branch 'upstream/master' into force-leader-on-h…

8a7733b

…ibernate

Connor1996 force-pushed the force-leader-on-hibernate branch from 67ec4bf to a41677f Compare April 15, 2022 06:20

BusyJay reviewed Apr 15, 2022

View reviewed changes

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

fix potential overflow

7ebe095

Signed-off-by: Connor1996 <zbk602423539@gmail.com>

BusyJay reviewed Apr 18, 2022

View reviewed changes

v01dstar reviewed Apr 19, 2022

View reviewed changes

Connor1996 requested a review from BusyJay April 19, 2022 06:26

address comment and add a test case

94449b7

Signed-off-by: Connor1996 <zbk602423539@gmail.com>

Connor1996 force-pushed the force-leader-on-hibernate branch from 9be6b92 to 94449b7 Compare April 19, 2022 06:27

BusyJay approved these changes Apr 19, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 19, 2022

v01dstar approved these changes Apr 20, 2022

View reviewed changes

tabokie approved these changes Apr 20, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 20, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 20, 2022

ti-chi-bot added 3 commits April 20, 2022 11:58

Merge branch 'master' into force-leader-on-hibernate

669be0a

Merge branch 'master' into force-leader-on-hibernate

6557a95

Merge branch 'master' into force-leader-on-hibernate

2ec3701

ti-chi-bot merged commit 8b509a7 into tikv:master Apr 20, 2022

Connor1996 deleted the force-leader-on-hibernate branch April 20, 2022 11:14

This was referenced May 18, 2022

Cherry pick unsafe recovery related PRs pingcap/tidb-engine-ext#62

Open

Cherry pick unsafe recovery related PRs pingcap/tidb-engine-ext#63

Merged

overvenus mentioned this pull request Jul 12, 2023

[Dynamic Region] Support online recovery for raftstore-v2 #15108

Closed

6 tasks

Conversation

Connor1996 commented Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Connor1996 commented Apr 15, 2022

Uh oh!

v01dstar Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

Connor1996 Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v01dstar Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

Connor1996 Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

v01dstar Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

Connor1996 Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot commented Apr 20, 2022

Uh oh!

Connor1996 commented Apr 20, 2022

Uh oh!

ti-chi-bot commented Apr 20, 2022

Uh oh!

ti-chi-bot commented Apr 20, 2022

Uh oh!

ti-chi-bot commented Apr 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Connor1996 commented Apr 15, 2022 •

edited

Loading

ti-chi-bot commented Apr 15, 2022 •

edited

Loading

Connor1996 Apr 15, 2022 •

edited

Loading