Add external state support #18

tylertreat · 2017-07-07T21:42:26Z

Add support for external state (log) to influence leader voting. This,
in effect, implements lastLogIndex and lastLogTerm sent on RequestVote
RPCs from the Raft paper. This works by exposing two callbacks: one that
calls into the user on RequestVote to get the candidate's state and one
that calls into the user upon receiving a RequestVote to determine if a
vote should be granted based on comparing the logs.

From Raft:

Reply false if term < currentTerm (§5.1)
If votedFor is null or candidateId, and candidate’s log is at
least as up-to-date as receiver’s log, grant vote (§5.2, §5.4)

@nats-io/core

Add support for external state (log) to influence leader voting. This, in effect, implements lastLogIndex and lastLogTerm sent on RequestVote RPCs from the Raft paper. This works by exposing two callbacks: one that calls into the user on RequestVote to get the candidate's state and one that calls into the user upon receiving a RequestVote to determine if a vote should be granted based on comparing the logs. From Raft: 1. Reply false if term < currentTerm (§5.1) 2. If votedFor is null or candidateId, and candidate’s log is at least as up-to-date as receiver’s log, grant vote (§5.2, §5.4)

coveralls · 2017-07-07T21:44:13Z

Coverage decreased (-0.1%) to 90.765% when pulling 433585a on log_state into 4629e3c on master.

coveralls · 2017-07-07T21:54:13Z

Coverage decreased (-0.1%) to 90.765% when pulling 23bf473 on log_state into 4629e3c on master.

kozlovic

Marking as requesting changes, but could be approve if we don't find a better name for the new handler/APIs.

kozlovic · 2017-07-07T22:07:13Z

chan_handler.go


 package graft

 // ChanHandler is a convenience handler when a user wants to simply use
 // channels for the async handling of errors and state changes.
 type ChanHandler struct {
+	LogPositionHandler


Apply to everything else in the PR: Is Log a good choice here? It may imply log replication, which as you know is not done in graft. Would something about vote or state be more appropriate?

Agreed, the name might be confusing. I decided against "state" because it was also confusing. There are already a lot of references to "state" in graft with respect to node state changes.

I wish I could think of something better, but can't. There isn't full RAFT spec log replication, but the value passed is actually the position in the log in the context of leadership election. If not a name change, I think a comment to that effect might help clarify.

kozlovic · 2017-07-07T22:08:47Z

handler_test.go

@@ -43,7 +58,8 @@ func TestStateChangeHandler(t *testing.T) {
 	// Use ChanHandler
 	scCh := make(chan StateChange)
 	errCh := make(chan error)
-	chHand := NewChanHandler(scCh, errCh)
+	lpHandler := &logPositionHandler{}


If you don't use lpHandler elsewhere, maybe you could pass &logPositionHandler{} directly in the NewChanHandler() call. True for 2 other occurrences in this test file.

kozlovic · 2017-07-07T22:09:24Z

node.go

@@ -78,8 +80,24 @@ type ClusterInfo struct {
 	Size int
 }

+// LogPositionHandler is used to interrogate the state of the log.
+type LogPositionHandler interface {


Not sold on the name. Again, Log may not be the best choice here, but not sure what to call it yet.

kozlovic · 2017-07-07T22:11:08Z

node.go

 		n.rpc.SendVoteResponse(vreq.Candidate, deny)
 		return false
 	}

-	// Save state flag


Note that the test above is vreq.Term < n.term and saveState was previously set to true if vreq.Term > n.term, which means that if they were equal, we would not save the state. You are changing that. Is it intentional?

If we send a vote response, the node's state changes. Before, this only happened if the term was higher, but now it also happens if term is equal but "log" is higher. My test was failing because the node state wasn't being persisted in this case. I guess we can still avoid saving if term is equal and the "log" is equal?

Not sure. The point was to make sure that the change was intentional, which I understand it is.

If term is equal and log is equal, we can avoid saving, but IMO just keep it simple and safe. How often are we expecting this to happen?

@ColinSullivan1 I agree, I don't think the optimization is worth the added complexity.

ColinSullivan1 · 2017-07-07T22:47:29Z

chan_handler.go

@@ -20,10 +22,11 @@ type StateChange struct {
 	To State
 }

-func NewChanHandler(scCh chan<- StateChange, errCh chan<- error) *ChanHandler {
+func NewChanHandler(logHandler LogPositionHandler, scCh chan<- StateChange, errCh chan<- error) *ChanHandler {


Since you are here, can you comment this API?

Thinking about it, what would you think about a new creation API, or a way to set the LogPositionHandler? A default handler could provide today's behavior, and we'd keep backward compatibility with existing users.

If possible and not too awkward, that would be best indeed. If not using new API, old behavior. Internally, it means that we would not invoke the callback.

Or the default handler callback can treat it as equal log positions (grant).

petemiron

Minor comment on name/logIndex size. Still need to run through tests.

petemiron · 2017-07-08T16:02:28Z

handler_test.go

@@ -12,6 +13,20 @@ import (
 	"github.com/nats-io/graft/pb"
 )

+type logPositionHandler struct {
+	logIndex uint32


Should we just use currentSequence instead of logIndex? Also, should this be at least a uint64?

Note that this is a test handler.

coveralls · 2017-07-10T17:57:20Z

Coverage decreased (-0.3%) to 90.625% when pulling 6692c9d on log_state into 4629e3c on master.

petemiron

LGTM. Tested and reviewed code.

ColinSullivan1 · 2017-07-12T17:36:52Z

node.go

-			return true
-		}
+	// Write our state.
+	if err := n.writeState(); err != nil {


I know a lot has gone into this. I wonder if writing the state should be the responsibility of the state machine interface implementor. wdyt?

This is writing state needed for leadership election, so seems like it should be graft's responsibility?

I do see your point, but was thinking that users may want a way to persist state themselves.

We could provide a hook that calls out to user code (via the StateMachineHandler) to signal them to persist. I think that could be added later though.

ColinSullivan1

LGTM, tests passed for me. I left a comment regarding writing state - IMO that could be addressed here or in another PR.

derekcollison · 2017-07-12T18:58:34Z

We should be looking at IO performance at some point two, for each shared state update we should be doing one write IMO, not multiple.

tylertreat added 2 commits July 7, 2017 16:33

Update copyrights

433585a

Fix comment in test

23bf473

kozlovic requested changes Jul 7, 2017

View reviewed changes

ColinSullivan1 reviewed Jul 7, 2017

View reviewed changes

petemiron reviewed Jul 8, 2017

View reviewed changes

Rename LogPositionHandler to StateMachineHandler

6692c9d

kozlovic approved these changes Jul 10, 2017

View reviewed changes

petemiron approved these changes Jul 12, 2017

View reviewed changes

ColinSullivan1 reviewed Jul 12, 2017

View reviewed changes

ColinSullivan1 approved these changes Jul 12, 2017

View reviewed changes

tylertreat merged commit ab0519d into master Jul 12, 2017

tylertreat deleted the log_state branch July 12, 2017 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add external state support #18

Add external state support #18

tylertreat commented Jul 7, 2017

coveralls commented Jul 7, 2017

coveralls commented Jul 7, 2017

kozlovic left a comment

kozlovic Jul 7, 2017

tylertreat Jul 7, 2017 •

edited

ColinSullivan1 Jul 7, 2017

kozlovic Jul 7, 2017

kozlovic Jul 7, 2017

kozlovic Jul 7, 2017

tylertreat Jul 7, 2017 •

edited

kozlovic Jul 7, 2017

ColinSullivan1 Jul 7, 2017

tylertreat Jul 8, 2017

ColinSullivan1 Jul 7, 2017

ColinSullivan1 Jul 7, 2017

kozlovic Jul 7, 2017

ColinSullivan1 Jul 7, 2017

petemiron left a comment

petemiron Jul 8, 2017

tylertreat Jul 8, 2017

coveralls commented Jul 10, 2017

petemiron left a comment

ColinSullivan1 Jul 12, 2017 •

edited

tylertreat Jul 12, 2017

ColinSullivan1 Jul 12, 2017

tylertreat Jul 12, 2017

ColinSullivan1 left a comment

derekcollison commented Jul 12, 2017

Add external state support #18

Add external state support #18

Conversation

tylertreat commented Jul 7, 2017

coveralls commented Jul 7, 2017

coveralls commented Jul 7, 2017

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylertreat Jul 7, 2017 • edited

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

tylertreat Jul 7, 2017 • edited

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

petemiron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 10, 2017

petemiron left a comment

Choose a reason for hiding this comment

ColinSullivan1 Jul 12, 2017 • edited

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

ColinSullivan1 left a comment

Choose a reason for hiding this comment

derekcollison commented Jul 12, 2017

tylertreat Jul 7, 2017 •

edited

tylertreat Jul 7, 2017 •

edited

ColinSullivan1 Jul 12, 2017 •

edited