Skip to content

Commit

Permalink
storage: harden getQuorumIndex
Browse files Browse the repository at this point in the history
It wasn't looking at the progress.State, which should be harmless
but better not to trust that the Match field is correctly populated
for followers in probing status.

Note that there's a potential behavior change here: if a follower
needs a snapshot, it will have a Match field. But if we're computing
a quorum index, we implicitly assume that progress can be made from
that index since a quorum of followers "has it". A follower which
needs a snapshot is not able to help out with progress until it
has been caught up, so including it in the quorum index is not
beneficial.

Release note: None
  • Loading branch information
tbg committed Feb 7, 2019
1 parent ab0b272 commit 328fadc
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 2 deletions.
6 changes: 5 additions & 1 deletion pkg/storage/raft_log_queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,11 @@ func computeTruncateDecision(input truncateDecisionInput) truncateDecision {
func getQuorumIndex(raftStatus *raft.Status) uint64 {
match := make([]uint64, 0, len(raftStatus.Progress))
for _, progress := range raftStatus.Progress {
match = append(match, progress.Match)
if progress.State == raft.ProgressStateReplicate {
match = append(match, progress.Match)
} else {
match = append(match, 0)
}
}
sort.Sort(uint64Slice(match))
quorum := computeQuorum(len(match))
Expand Down
13 changes: 12 additions & 1 deletion pkg/storage/raft_log_queue_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,24 @@ func TestGetQuorumIndex(t *testing.T) {
Progress: make(map[uint64]raft.Progress),
}
for j, v := range c.progress {
status.Progress[uint64(j)] = raft.Progress{Match: v}
status.Progress[uint64(j)] = raft.Progress{State: raft.ProgressStateReplicate, Match: v}
}
quorumMatchedIndex := getQuorumIndex(status)
if c.expected != quorumMatchedIndex {
t.Fatalf("%d: expected %d, but got %d", i, c.expected, quorumMatchedIndex)
}
}

// Verify that only replicating followers are taken into account (i.e. others
// are treated as Match == 0).
status := &raft.Status{
Progress: map[uint64]raft.Progress{
1: {State: raft.ProgressStateReplicate, Match: 100},
2: {State: raft.ProgressStateSnapshot, Match: 100},
3: {State: raft.ProgressStateReplicate, Match: 90},
},
}
assert.Equal(t, uint64(90), getQuorumIndex(status))
}

func TestComputeTruncateDecision(t *testing.T) {
Expand Down

0 comments on commit 328fadc

Please sign in to comment.