-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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: add handleHeartbeat #1740
Conversation
29e76b1
to
24a9bfe
Compare
} | ||
l.committed = tocommit | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a specific test for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
we need to make corresponded change at https://github.com/coreos/etcd/blob/master/raft/raft.go#L432, which handles heartbeat response before. |
HardState: pb.HardState{Term: 2}, | ||
raftLog: &raftLog{committed: 0, ents: []pb.Entry{{}, {Term: 1}, {Term: 2}, {Term: 3}}}, | ||
} | ||
sm.raftLog.commitTo(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sm.raftLog.commitTo(commit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
handleHeartbeat commits to the commit index in the message. It never decreases the commit index of the raft state machine.
24a9bfe
to
bd4cfa2
Compare
done. I am not going to change the heartbeat response handling logic at leader side, since we are not sure if we will add the heartbeat response back soon. We can revisit all these sort of thing when we introduce the |
if l.committed < tocommit { | ||
l.committed = tocommit | ||
} | ||
l.commitTo(min(committed, lastnewi)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bdarnell You probably need to do a rebase due to this change. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change will be a problem, actually, since I haven't really touched the commit logic. I have, however, had to do a number of non-trivial merges for changes around stableTo(). Once I finish up the snapshot stuff I'm currently working on I think my branch will be ready to merge back into the mainline.
lgtm |
handleHeartbeat commits to the commit index in the message. It never decreases the
commit index of the raft state machine.