Replace inflight tracker with commitment tracker (retargeted) #102
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
inflight.go used to have two jobs: it buffered log futures until they were committed, and it marked new log entries committed. This second job is what commitment.go does, while raft.go now tracks its own uncommitted log futures. Inflight used to track the replication factor of each log future/entry independently. It kept a count of how many servers stored that entry, and how many were needed to mark it committed. The primary issue is that a simple count of how many servers store an entry is fragile with respect to membership changes, where the identity of the voting servers might change over time. It was also a fair amount of bookkeeping to track this information per entry. The new 'commitment' module instead tracks the match index for each server: the last entry that the server has on disk that agrees with this leader. This has several advantages: - No information is kept per log entry. - Only the latest known membership configuration is used to mark entries committed, as described in the Raft paper/dissertation. - Servers that aren't voters are simply ignored for the purpose of commitment. This is useful for leaders that remove themselves today but would also be useful for more general non-voting members in the future. Because no information is kept per log entry, the log futures can now be state kept local to the leader goroutine, without any need for locking. This simplified the interface to commitment.go (relative to inflight.go) significantly, while only adding a few lines here and there to raft.go.
| r.leaderState.inflight = newInflight(r.leaderState.commitCh) | ||
| r.leaderState.commitment = newCommitment(r.leaderState.commitCh, | ||
| append([]string{r.localAddr}, r.peers...), | ||
| /* first index that may be committed in this term: */ r.getLastIndex()+1) |
There was a problem hiding this comment.
Could you add the comment as trailing instead of leading?
|
Minor nit, but LGTM |
|
Looks good! |
slackpad
added a commit
that referenced
this pull request
Mar 21, 2016
Replace inflight tracker with commitment tracker (retargeted)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Retargeted #87 onto the issue-84-integration branch. The original commit (259e54d) is the same as before, only rebased. The subsequent commits address the comments from PR #87. This should probably be squashed into one commit when merging.
The only comment I didn't take from #87 was @armon's about switching to an RWMutex. (I don't plan to do that unless you insist.)