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

raft: do not attach term to MsgReadIndex #6749

Merged
merged 1 commit into from
Oct 29, 2016
Merged

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Oct 28, 2016

Fix #6744.

MsgReadIndex, as MsgProp, is to be forwarded to leader.
So we should treat it as local message.

@xiang90

nt.send(raftpb.Message{From: 3, To: 3, Type: raftpb.MsgHup})

// to old leader r1
r2.Step(raftpb.Message{
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not correct. r2 should step message sent to itself (To:2). It should not step a message sent to 1.

@@ -190,6 +190,44 @@ func TestNodeReadIndex(t *testing.T) {
}
}

// TestNodeReadIndexToOldLeader ensures that raftpb.MsgReadIndex to old leader
// gets forwarded to the new leader and 'send' method does not attach its term.
func TestNodeReadIndexToOldLeader(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

probably all we want to test here is:

  • elect 1 as leader
  • send readindex request to 2
  • verify 2 forwards this message to 1 with term not set
  • send readindex request to 3
  • verify 3 forwards this message to 1 with term not set as well.
  • now elect 3 as leader
  • let 1 steps the two messages previously we got from 2, 3
  • verify 1 forwards these messages again to the new leader 3

@gyuho
Copy link
Contributor Author

gyuho commented Oct 29, 2016

@xiang90 Just fixed test. PTAL. Thanks!

@xiang90
Copy link
Contributor

xiang90 commented Oct 29, 2016

@gyuho Will the test panics (or fails) without the patch?

@gyuho
Copy link
Contributor Author

gyuho commented Oct 29, 2016

@xiang90 Yes it detects the term attached to MsgReadIndex

--- FAIL: TestNodeReadIndexToOldLeader (0.00s)
node_test.go:221: r2.msgs[0] expected {Type:MsgReadIndex To:1 From:2 Term:0 LogTerm:0 Index:0 Entries:[{Term:0 Index:0 Type:EntryNormal Data:[116 101 115 116 100 97 116 97] XXX_unrecognized:[]}] Commit:0 Snapshot:{Data:[] Metadata:{ConfState:{Nodes:[] XXX_unrecognized:[]} Index:0 Term:0 XXX_unrecognized:[]} XXX_unrecognized:[]} Reject:false RejectHint:0 Context:[] XXX_unrecognized:[]}, got {Type:MsgReadIndex To:1 From:2 Term:1 LogTerm:0 Index:0 Entries:[{Term:0 Index:0 Type:EntryNormal Data:[116 101 115 116 100 97 116 97] XXX_unrecognized:[]}] Commit:0 Snapshot:{Data:[] Metadata:{ConfState:{Nodes:[] XXX_unrecognized:[]} Index:0 Term:0 XXX_unrecognized:[]} XXX_unrecognized:[]} Reject:false RejectHint:0 Context:[] XXX_unrecognized:[]}
FAIL
exit status 1
FAIL github.com/coreos/etcd/raft 0.003s

@xiang90
Copy link
Contributor

xiang90 commented Oct 29, 2016

LGTM

Fix etcd-io#6744.

MsgReadIndex, as MsgProp, is to be forwarded to leader.
So we should treat it as local message.
@gyuho
Copy link
Contributor Author

gyuho commented Oct 29, 2016

Thanks. Merging in greens.

@gyuho gyuho merged commit 4a08678 into etcd-io:master Oct 29, 2016
@gyuho gyuho deleted the raft-prevote branch October 29, 2016 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants