From e55836365daeb69af20e018ac34313abec861b34 Mon Sep 17 00:00:00 2001
From: Tobias Schottdorf <tobias.schottdorf@gmail.com>
Date: Sat, 8 Jun 2019 22:07:19 +0200
Subject: [PATCH] raft: clarify successful snapshot path

Receiving a non-rejecting MsgApp is a standard way of learning that
a follower has successfully applied a snapshot. Confusingly this
was dubbed as "aborting a snapshot" by the code.
---
 raft/raft.go             |  8 ++++++--
 raft/tracker/progress.go | 15 +++++++--------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/raft/raft.go b/raft/raft.go
index 70da7d628dfb..e90cc16ae0ae 100644
--- a/raft/raft.go
+++ b/raft/raft.go
@@ -1044,8 +1044,8 @@ func stepLeader(r *raft, m pb.Message) error {
 				switch {
 				case pr.State == tracker.StateProbe:
 					pr.BecomeReplicate()
-				case pr.State == tracker.StateSnapshot && pr.NeedSnapshotAbort():
-					r.logger.Debugf("%x snapshot aborted, resumed sending replication messages to %x [%s]", r.id, m.From, pr)
+				case pr.State == tracker.StateSnapshot && pr.Match >= pr.PendingSnapshot:
+					r.logger.Debugf("%x recovered from needing snapshot, resumed sending replication messages to %x [%s]", r.id, m.From, pr)
 					// Transition back to replicating state via probing state
 					// (which takes the snapshot into account). If we didn't
 					// move to replicating state, that would only happen with
@@ -1112,6 +1112,10 @@ func stepLeader(r *raft, m pb.Message) error {
 		if pr.State != tracker.StateSnapshot {
 			return nil
 		}
+		// TODO(tbg): this code is very similar to the snapshot handling in
+		// MsgAppResp above. In fact, the code there is more correct than the
+		// code here and should likely be updated to match (or even better, the
+		// logic pulled into a newly created Progress state machine handler).
 		if !m.Reject {
 			pr.BecomeProbe()
 			r.logger.Debugf("%x snapshot succeeded, resumed sending replication messages to %x [%s]", r.id, m.From, pr)
diff --git a/raft/tracker/progress.go b/raft/tracker/progress.go
index 6912bd3e33f0..251cf302b02a 100644
--- a/raft/tracker/progress.go
+++ b/raft/tracker/progress.go
@@ -16,8 +16,13 @@ package tracker
 
 import "fmt"
 
-// Progress represents a follower’s progress in the view of the leader. Leader maintains
-// progresses of all followers, and sends entries to the follower based on its progress.
+// Progress represents a follower’s progress in the view of the leader. Leader
+// maintains progresses of all followers, and sends entries to the follower
+// based on its progress.
+//
+// NB(tbg): Progress is basically a state machine whose transitions are mostly
+// strewn around `*raft.raft`. Additionally, some fields are only used when in a
+// certain State. All of this isn't ideal.
 type Progress struct {
 	Match, Next uint64
 	// State defines how the leader should interact with the follower.
@@ -196,12 +201,6 @@ func (pr *Progress) IsPaused() bool {
 	}
 }
 
-// needSnapshotAbort returns true if snapshot progress's Match
-// is equal or higher than the pendingSnapshot.
-func (pr *Progress) NeedSnapshotAbort() bool {
-	return pr.State == StateSnapshot && pr.Match >= pr.PendingSnapshot
-}
-
 func (pr *Progress) String() string {
 	return fmt.Sprintf("next = %d, match = %d, state = %s, waiting = %v, pendingSnapshot = %d, recentActive = %v, isLearner = %v",
 		pr.Next, pr.Match, pr.State, pr.IsPaused(), pr.PendingSnapshot, pr.RecentActive, pr.IsLearner)