Skip to content

raftstore: fix lost request vote messages during split#8454

Merged
overvenus merged 5 commits intotikv:masterfrom
overvenus:fix-999-10s
Aug 18, 2020
Merged

raftstore: fix lost request vote messages during split#8454
overvenus merged 5 commits intotikv:masterfrom
overvenus:fix-999-10s

Conversation

@overvenus
Copy link
Member

@overvenus overvenus commented Aug 14, 2020

What problem does this PR solve?

Fix #8456, request vote messages are lost during split, which is introduced by #8084.

This PR improve TiDB 99.999% Txn duration from 15s to 1s.

image

image

What is changed and how it works?

Cache vote message into pending votes, so that new peer can response ASAP.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Integration test

Release note

  • Fix lost request vote messages during split.

Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus overvenus added sig/raft Component: Raft, RaftStore, etc. type/bugfix This PR fixes a bug. labels Aug 14, 2020
Signed-off-by: Neil Shen <overvenus@gmail.com>
@NingLin-P
Copy link
Member

NingLin-P commented Aug 14, 2020

Rest LGTM

@ti-srebot
Copy link
Contributor

@NingLin-P,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: raft(slack).

@BusyJay BusyJay requested a review from gengliqi August 14, 2020 07:32
Copy link
Member

@gengliqi gengliqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! The cost of losing request vote messages is very high.
Since the check_msg has verified that message is newer and a new peer can be created, I think it's better to save this message if maybe_create_peer returns Ok(false)?
Also, remember to remove some code in maybe_create_peer that adds this msg to pending_votes.

Signed-off-by: Neil Shen <overvenus@gmail.com>
@zhangjinpeng87
Copy link
Member

Better to open an individual issue to describe the bug.

Copy link
Member

@gengliqi gengliqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_first_vote seems a little strange. But I also have no good idea😑.

Comment on lines +1531 to +1533
}
}
is_first_vote
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}
is_first_vote
}
is_first_vote
}
false

Signed-off-by: Neil Shen <overvenus@gmail.com>
Copy link
Member

@gengliqi gengliqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 18, 2020
@ti-srebot
Copy link
Contributor

@NingLin-P,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: raft(slack).

@overvenus overvenus merged commit a27a28b into tikv:master Aug 18, 2020
@overvenus overvenus deleted the fix-999-10s branch August 18, 2020 08:24
gengliqi added a commit to gengliqi/tikv that referenced this pull request Jan 26, 2021
Signed-off-by: gengliqi <gengliqiii@gmail.com>
ti-chi-bot pushed a commit that referenced this pull request Jul 13, 2021
…rrectly (#9584)

* cherry pick #8084 #8454 #9495

Signed-off-by: gengliqi <gengliqiii@gmail.com>

* fix compile error

Signed-off-by: gengliqi <gengliqiii@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/raft Component: Raft, RaftStore, etc. status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Request vote messages are lost during split

5 participants