Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

raftstore: reject transfer leader to the recently added peers #3878

Merged
merged 16 commits into from
Dec 7, 2018

Conversation

nolouch
Copy link
Contributor

@nolouch nolouch commented Dec 5, 2018

What have you changed? (mandatory)

The apply state not consistent with the leader, transfer leader to newly added peer probable failed . reject transfer leader to the recently added peers to prevent transferring failed.
This is a quick fix, need remove after tikv/raft-rs#144 sloved.
ref: #3819

What are the type of the changes? (mandatory)

  • Improvemen

How has this PR been tested? (mandatory)

unit test.

manua test:
master(before) vs bransh(after):
prepare: add a special hot scheduler that move hot region and transfer the leader to the newly added hot peer. run oltp test.
result:
deepinscreenshot_select-area_20181206131739

Signed-off-by: nolouch <nolouch@gmail.com>
Signed-off-by: nolouch <nolouch@gmail.com>
@nolouch nolouch force-pushed the saft-transfer branch 2 times, most recently from 41cc231 to d87203e Compare December 5, 2018 03:42
Signed-off-by: nolouch <nolouch@gmail.com>
src/raftstore/store/config.rs Outdated Show resolved Hide resolved
src/raftstore/store/config.rs Outdated Show resolved Hide resolved
src/raftstore/store/peer.rs Outdated Show resolved Hide resolved
src/raftstore/store/peer.rs Outdated Show resolved Hide resolved
@siddontang
Copy link
Contributor

em, can the leader know the applied index of the followers so that we can do a more accurate control?

@nolouch
Copy link
Contributor Author

nolouch commented Dec 5, 2018

@siddontang Currently, the apply index can only be known by itself. If need precise control, should change it in the raft library. This is a quick fix, and the long-term solution maybe is this.

@ngaut
Copy link
Member

ngaut commented Dec 5, 2018

I prefer to do a quick fix for now. And make it better later.

@rleungx rleungx added the priority/critical Priority: Critical label Dec 5, 2018
Signed-off-by: nolouch <nolouch@gmail.com>
Signed-off-by: nolouch <nolouch@gmail.com>
Signed-off-by: nolouch <nolouch@gmail.com>
@nolouch nolouch added the sig/raft Component: Raft, RaftStore, etc. label Dec 5, 2018
@siddontang
Copy link
Contributor

This still has the risk that the learner drops the transfer leader message if the learner applies too slow.

/cc @BusyJay

@BusyJay
Copy link
Member

BusyJay commented Dec 5, 2018

Yes, it does. But it should be hardly possible.

Signed-off-by: nolouch <nolouch@gmail.com>
Signed-off-by: nolouch <nolouch@gmail.com>
@ngaut
Copy link
Member

ngaut commented Dec 6, 2018

Good job.

@nolouch
Copy link
Contributor Author

nolouch commented Dec 6, 2018

PTAL @BusyJay @zhangjinpeng1987

Signed-off-by: nolouch <nolouch@gmail.com>
@@ -893,6 +893,10 @@ impl<T: Transport, C: PdClient> Store<T, C> {
if p.is_leader() {
p.peers_start_pending_time.push((id, now));
}
// Add peer or promote the learner
if !peer.get_is_learner() {
Copy link
Member

Choose a reason for hiding this comment

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

I think only leader needs to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe transfer From A to B then to Cnew.

src/raftstore/store/peer.rs Outdated Show resolved Hide resolved
src/raftstore/store/peer.rs Outdated Show resolved Hide resolved
zhangjinpeng87
zhangjinpeng87 previously approved these changes Dec 7, 2018
Copy link
Member

@zhangjinpeng87 zhangjinpeng87 left a comment

Choose a reason for hiding this comment

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

LGTM

src/raftstore/store/peer.rs Outdated Show resolved Hide resolved
tests/integrations/raftstore/test_conf_change.rs Outdated Show resolved Hide resolved
tests/integrations/raftstore/test_conf_change.rs Outdated Show resolved Hide resolved
Signed-off-by: nolouch <nolouch@gmail.com>
Signed-off-by: nolouch <nolouch@gmail.com>
Copy link
Member

@zhangjinpeng87 zhangjinpeng87 left a comment

Choose a reason for hiding this comment

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

LGTM

@zhangjinpeng87 zhangjinpeng87 merged commit f672088 into tikv:master Dec 7, 2018
nolouch added a commit to nolouch/tikv that referenced this pull request Dec 8, 2018
nolouch added a commit to nolouch/tikv that referenced this pull request Dec 8, 2018
nolouch added a commit that referenced this pull request Dec 8, 2018
@nolouch nolouch deleted the saft-transfer branch December 12, 2018 11:25
nolouch added a commit to nolouch/tikv that referenced this pull request Dec 14, 2018
nolouch added a commit to nolouch/tikv that referenced this pull request Dec 14, 2018
rleungx pushed a commit that referenced this pull request Dec 14, 2018
…#3929)

* raftstore: reject transfer leader to the recently added peers (#3878)
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/critical Priority: Critical sig/raft Component: Raft, RaftStore, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants