raft: add learner #8605

lishuai87 · 2017-09-25T16:40:31Z

Partly implement #8568, only add Nonvoter support.

for protobuf, maybe we can add a version for pb.ConfState.

lishuai87 · 2017-09-27T03:54:56Z

@xiang90 @siddontang PTAL

xiang90 · 2017-09-28T23:18:19Z

sorry for the delay. i will try to give this a review this week.

xiang90 · 2017-09-29T20:14:42Z

raft/raftpb/raft.proto

 message ConfState {
-	repeated uint64 nodes = 1;
+	repeated uint64 nodes   = 1;
+	repeated Server servers = 2;


can we make this simpler by just having

repeated uint64 nodes = 1; repeated uint64 learners= 2;

?

we may also have stagings after addVoter feature

i do not like the idea of staging server. i would rather let the application itself promote the leader to a node explicitly.

basically, the less raft layer needs to care about the better.

xiang90 · 2017-09-29T20:16:03Z

can we rename Nonvoter to Learners?

also i would like to simplify the code path and minimize code change by having an explicit list of learners instead of a mixed list of Servers (which can be peer or non-voter).

xiang90 · 2017-09-30T19:58:04Z

the learner should always stay as follower state, and never try to promote or vote, right? where do we prevent that from happening?

lishuai87 · 2017-10-01T03:18:05Z

the learner should always stay as follower state, and never try to promote or vote, right? where do we prevent that from happening?

the promotable will always false for learner:

func (r *raft) promotable() bool {
 	_, ok := r.prs[r.id]
	return ok && !r.isLearner
 }

learner should be able to vote. because if we addNode a learner, the voters may think that node is voter, but that node may think its still learner.
for example,

cluster has [A, B, C], with A is leader,
addLearner D. the majority is still 2.
addNode D. [A, B, C] both apply the confchange, but D don't recv the append log due to log lag or network jitter.
A down. [B, C] is voter, they think D is voter too, the majority is 3. but D think itself is learner.
if learner can't vote, the cluster will hang until A recover. if learner can vote, the cluster will ok immediately.

xiang90 · 2017-10-03T01:21:42Z

learner should be able to vote. because if we addNode a learner, the voters may think that node is voter, but that node may think its still learner.

The same thing happens with normal add node case. There are two motivations for adding learner:

scale up read request when consistency can be relaxed
avoid slow member to disrupt the cluster (only promote to normal member when it can catch up)

What you are trying to do is to enhance 2. I would like to know why 2 itself is not strong enough.

Do you have a specific use case or failure mode in mind?

lishuai87 · 2017-10-04T03:52:34Z

In our case, we have 3 nodes in one raft group. If we want replace one node, we will add a new node first, then remove the old one.
But if a node down, the replace operation can't run as usual. Because if we add a new node first, the majority will be 3, and client requests will hang/timeout until the new node catch up.

siddontang · 2017-10-09T01:28:58Z

Assume we have 3 nodes, A, B and C, and want to replace A with D, so now we can:

Add learner D
Remove A
Add D

The corner case here is after 2, we have only B, C and if any one of them fails, the cluster can't work.

If the learner D can vote, this is not a problem, but here we have another corner case, after 1, we have A, B, C, D, A and D are on the same node, so if the node crashes, the cluster can't work too.

We have the same problem when we do:

Add learner D
Add D
Remove A

I don't think the learner mechanism can fix this corner case, maybe only Raft Membership Change can do. So I suggest keeping things simple and letting learner don't be able to vote.

xiang90 · 2017-10-09T02:53:06Z

I would say keep it as simple as possible for now. if the corner case does happen in reality or if we have a more concrete real world use case and failure case, we can improve it.

xiang90 · 2017-10-09T02:54:19Z

Just to be more clear, after adding the learner concept, we already largely improve the reliability of membership change process. We will only promote a learner to a member if it can catch up.

siddontang · 2017-10-10T03:52:23Z

raft/raft.go

-	mis := make(uint64Slice, 0, len(r.prs))
-	for id := range r.prs {
-		mis = append(mis, r.prs[id].Match)
+	mis := make(uint64Slice, 0, r.voterCount())


seem we can still use len(r.prs) to reduce the extra learner check in voterCount.

siddontang · 2017-10-11T10:03:45Z

PTAL @xiang90

xiang90 · 2017-10-11T17:27:03Z

@siddontang i would like to take another look after @javaforfun disables the vote of the learner.

siddontang · 2017-10-12T00:24:23Z

Hi @xiang90

Seem the learner can't vote now, see https://github.com/coreos/etcd/pull/8605/files#diff-a8629a0eb6da1c6c3805111f1f1af85eR376

lishuai87 · 2017-10-12T02:41:21Z

@xiang90 If learner can't vote, how to prevent this problem? #8605 (comment)

xiang90 · 2017-10-12T02:46:02Z

@javaforfun we are not going to solve the problem you brought up for now. we should focus on solving:

scale up read request when consistency can be relaxed
avoid slow member to disrupt the cluster (only promote to normal member when it can catch up)

siddontang · 2017-10-12T02:50:06Z

I agree with @xiang90, here we don't need a 100% solution for ConfChange.

siddontang · 2017-10-16T00:24:10Z

PTAL @xiang90

xiang90 · 2017-10-17T16:44:52Z

raft/raft.go

@@ -116,6 +116,8 @@ type Config struct {
 	// used for testing right now.
 	peers []uint64

+	learners []uint64


doc string?

xiang90 · 2017-10-17T16:45:46Z

raft/raft.go

-	peers := c.peers
-	if len(cs.Nodes) > 0 {
-		if len(peers) > 0 {
+	voters := c.peers


use peers instead of voters. i do not want to create a new term for now.

xiang90 · 2017-10-17T16:46:35Z

raft/raft.go

-	for _, p := range peers {
-		r.prs[p] = &Progress{Next: 1, ins: newInflights(r.maxInflight)}
+	for _, n := range voters {
+		r.prs[n] = &Progress{Next: 1, ins: newInflights(r.maxInflight), isLearner: false}


revert this change. bool is false by default.

xiang90 · 2017-10-17T16:53:16Z

raft/raftpb/raft.proto

 }

 enum ConfChangeType {
 	ConfChangeAddNode    = 0;
 	ConfChangeRemoveNode = 1;
 	ConfChangeUpdateNode = 2;
+	ConfChangeAddLearner = 3;


ConfChangeAddLearnerNode

xiang90 · 2017-10-17T17:05:17Z

raft/raft.go

+		if _, has := r.prs[n]; has {
+			panic(fmt.Sprintf("cannot specify both Voter and Learner for node: %x", n))
+		}
+		r.prs[n] = &Progress{Next: 1, ins: newInflights(r.maxInflight), isLearner: true}


maybe it is easier to have a separate prs map for learners.

xiang90 · 2017-10-17T17:23:09Z

raft/raft.go

+	return true
+}
+
+func (r *raft) restoreNode(nodes []uint64, isLearner bool) {


probably restoreNode can take both nodes and learnerNodes as args.

xiang90 · 2017-10-17T17:23:57Z

raft/raft.go

 }

 // promotable indicates whether state machine can be promoted to leader,
-// which is true when its own id is in progress list.
+// which is true when its own id is in progress list and its not learner.


it is not a learner

xiang90 · 2017-10-17T17:24:30Z

raft/raft.go

 }

 // promotable indicates whether state machine can be promoted to leader,
-// which is true when its own id is in progress list.
+// which is true when its own id is in progress list and its not learner.
 func (r *raft) promotable() bool {
 	_, ok := r.prs[r.id]


if r.isLearer { return false } ...

is cleaner

xiang90 · 2017-10-17T17:25:58Z

raft/raft.go

+	r.addLearnerNode(id, true)
+}
+
+func (r *raft) addLearnerNode(id uint64, isLearner bool) {


addNodeOrLearnerNode.

or probably just duplicate code a little bit, and remove this func.

xiang90 · 2017-10-17T17:26:48Z

raft/raft.go

+			return
+		}
+		if isLearner {
+			// can only change Learner to Voter


xiang90 · 2017-10-18T15:17:57Z

raft/raft.go

+		}
+		if isLearner {
+			// can only change Learner to Voter
+			r.logger.Infof("%x ignore addLearner for %x [%s]", r.id, id, r.prs[id])


need to log the reason why addLearner is ignored.

siddontang · 2017-10-19T03:56:48Z

raft/raft.go

+	return count
+}
+
+func (r *raft) quorum() int { return r.voterCount()/2 + 1 }


This function is called frequently, maybe now we can use a variable to cache the quorum.

xiang90 · 2017-10-19T04:02:30Z

raft/raft_test.go

@@ -3278,7 +3388,7 @@ func entsWithConfig(configFunc func(*Config), terms ...uint64) *raft {
 	for i, term := range terms {
 		storage.Append([]pb.Entry{{Index: uint64(i + 1), Term: term}})
 	}
-	cfg := newTestConfig(1, []uint64{}, 5, 1, storage)
+	cfg := newTestConfig(1, []uint64{1, 2, 3, 4, 5}, 5, 1, storage)


why this is changed?

xiang90 · 2017-10-19T04:02:37Z

raft/raft_test.go

@@ -3293,7 +3403,7 @@ func entsWithConfig(configFunc func(*Config), terms ...uint64) *raft {
 func votedWithConfig(configFunc func(*Config), vote, term uint64) *raft {
 	storage := NewMemoryStorage()
 	storage.SetHardState(pb.HardState{Vote: vote, Term: term})
-	cfg := newTestConfig(1, []uint64{}, 5, 1, storage)
+	cfg := newTestConfig(1, []uint64{1, 2, 3, 4, 5}, 5, 1, storage)


why this is changed?

xiang90 · 2017-10-19T04:05:24Z

raft/raftpb/raft.proto

@@ -76,13 +76,15 @@ message HardState {
 }

 message ConfState {
-	repeated uint64 nodes = 1;
+	repeated uint64 nodes    = 1;  // Voters


remove this comment. voter is meaningless without any context. or we need to explain exactly what is nodes.

xiang90 · 2017-10-19T04:05:37Z

raft/raftpb/raft.proto

@@ -76,13 +76,15 @@ message HardState {
 }

 message ConfState {
-	repeated uint64 nodes = 1;
+	repeated uint64 nodes    = 1;  // Voters
+	repeated uint64 learners = 2;  // Nonvoters


same here. we need to explain what it nonvoters.

lishuai87 · 2017-10-19T08:19:44Z

maybe it is easier to have a separate prs map for learners.

we may need to handle the prs before merging this PR. also the prs in status struct, https://github.com/coreos/etcd/blob/master/raft/status.go#L30

type Status struct {
	ID uint64

	pb.HardState
	SoftState

	Applied  uint64
	Progress map[uint64]Progress

	LeadTransferee uint64
}

siddontang · 2017-10-20T01:50:06Z

maybe it is easier to have a separate prs map for learners.

Seem this can handle leaner more easily, but may introduce complexity in other places like in bcast.

/cc @xiang90

xiang90 · 2017-10-20T02:04:27Z

@siddontang

right. someone should give it a try at least to see if it is simpler.

xiang90 · 2017-11-01T22:31:38Z

closed in favor of #8751

lishuai87 added 2 commits September 26, 2017 00:33

raft: add non-voter

a91a51d

raft: promotable check raft suffrage

c098dbb

lishuai87 mentioned this pull request Sep 27, 2017

[WIP] raft: add non-voter tikv/tikv#2351

Closed

xiang90 self-assigned this Sep 28, 2017

xiang90 added the area/raft label Sep 28, 2017

xiang90 reviewed Sep 29, 2017

View reviewed changes

raft: use learner instead of suffrage

b159733

lishuai87 force-pushed the raft-non-voter branch from 498d3fe to b159733 Compare September 30, 2017 09:53

lishuai87 changed the title ~~raft: add non-voter~~ raft: add learner Sep 30, 2017

raft: add TestLearnerElectionTimeout

ee003a5

siddontang reviewed Oct 10, 2017

View reviewed changes

raft: ignore vote request for learner

aff1c50

xiang90 reviewed Oct 17, 2017

View reviewed changes

raft/raft.go

return

}

if isLearner {

// can only change Learner to Voter

Copy link

Contributor

xiang90 Oct 17, 2017

Choose a reason for hiding this comment

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

log here.

raft: ConfChangeAddLearnerNode

abb497a

xiang90 reviewed Oct 18, 2017

View reviewed changes

siddontang reviewed Oct 19, 2017

View reviewed changes

xiang90 reviewed Oct 19, 2017

View reviewed changes

raft: address comments

2d2c6b9

siddontang mentioned this pull request Oct 24, 2017

raft: add raft learner #8751

Merged

xiang90 closed this Nov 1, 2017

lishuai87 deleted the raft-non-voter branch November 12, 2017 03:00

raft: add learner #8605

raft: add learner #8605

Conversation

lishuai87 commented Sep 25, 2017

lishuai87 commented Sep 27, 2017 • edited

xiang90 commented Sep 28, 2017

Choose a reason for hiding this comment

lishuai87 Sep 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 commented Sep 29, 2017

xiang90 commented Sep 30, 2017

lishuai87 commented Oct 1, 2017

xiang90 commented Oct 3, 2017

lishuai87 commented Oct 4, 2017

siddontang commented Oct 9, 2017

xiang90 commented Oct 9, 2017

xiang90 commented Oct 9, 2017

Choose a reason for hiding this comment

siddontang commented Oct 11, 2017

xiang90 commented Oct 11, 2017

siddontang commented Oct 12, 2017

lishuai87 commented Oct 12, 2017

xiang90 commented Oct 12, 2017

siddontang commented Oct 12, 2017

siddontang commented Oct 16, 2017

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

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

Choose a reason for hiding this comment

lishuai87 commented Oct 19, 2017

siddontang commented Oct 20, 2017 • edited

xiang90 commented Oct 20, 2017

xiang90 commented Nov 1, 2017

lishuai87 commented Sep 27, 2017 •

edited

lishuai87 Sep 30, 2017 •

edited

siddontang commented Oct 20, 2017 •

edited