raft: send commit index only if necessary

This commit fixes one case of unnecessary MsgApp sends. The leader now checks that the follower's commit index is behind, and only then sends a commit index update. Signed-off-by: Pavel Kalinnikov <[email protected]>
etcd-io · Jan 25, 2024 · 8e5b410 · 8e5b410
1 parent 03790d5
commit 8e5b410
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 76 deletions.
diff --git a/raft.go b/raft.go
@@ -1465,8 +1465,8 @@ func stepLeader(r *raft, m pb.Message) error {
 				r.sendAppend(m.From)
 			}
 		} else {
-			pr.UpdateCommit(m.Commit)
 			oldPaused := pr.IsPaused()
+			pr.UpdateCommit(m.Commit)
 			// We want to update our tracking if the response updates our
 			// matched index or if the response can move a probing peer back
 			// into StateReplicate (see heartbeat_rep_recovers_from_probing.txt
@@ -1503,11 +1503,8 @@ func stepLeader(r *raft, m pb.Message) error {
 					// to respond to pending read index requests
 					releasePendingReadIndexMessages(r)
 					r.bcastAppend()
-				} else if oldPaused {
-					// If we were paused before, this node may be missing the
-					// latest commit index, so send it.
-					// TODO(pav-kv): remove this branch, and decide on sending the commit
-					// index update based on pr.Commit.
+				} else if oldPaused && r.id != m.From && pr.Commit < r.raftLog.committed {
+					// The node is potentially missing the latest commit index. Send it.
 					r.sendAppend(m.From)
 				}
 				// We've updated flow control information above, which may

diff --git a/raft_paper_test.go b/raft_paper_test.go
@@ -606,20 +606,21 @@ func TestFollowerCheckMsgApp(t *testing.T) {
 		term        uint64
 		index       uint64
 		windex      uint64
+		wcommit     uint64
 		wreject     bool
 		wrejectHint uint64
 		wlogterm    uint64
 	}{
 		// match with committed entries
-		{0, 0, 1, false, 0, 0},
-		{ents[0].Term, ents[0].Index, 1, false, 0, 0},
+		{0, 0, 1, 1, false, 0, 0},
+		{ents[0].Term, ents[0].Index, 1, 0, false, 0, 0},
 		// match with uncommitted entries
-		{ents[1].Term, ents[1].Index, 2, false, 0, 0},
+		{ents[1].Term, ents[1].Index, 2, 0, false, 0, 0},
 
 		// unmatch with existing entry
-		{ents[0].Term, ents[1].Index, ents[1].Index, true, 1, 1},
+		{ents[0].Term, ents[1].Index, ents[1].Index, 0, true, 1, 1},
 		// unexisting entry
-		{ents[1].Term + 1, ents[1].Index + 1, ents[1].Index + 1, true, 2, 2},
+		{ents[1].Term + 1, ents[1].Index + 1, ents[1].Index + 1, 0, true, 2, 2},
 	}
 	for i, tt := range tests {
 		storage := newTestMemoryStorage(withPeers(1, 2, 3))
@@ -632,7 +633,7 @@ func TestFollowerCheckMsgApp(t *testing.T) {
 
 		msgs := r.readMessages()
 		wmsgs := []pb.Message{
-			{From: 1, To: 2, Type: pb.MsgAppResp, Term: 2, Index: tt.windex, Reject: tt.wreject, RejectHint: tt.wrejectHint, LogTerm: tt.wlogterm},
+			{From: 1, To: 2, Type: pb.MsgAppResp, Term: 2, Index: tt.windex, Commit: tt.wcommit, Reject: tt.wreject, RejectHint: tt.wrejectHint, LogTerm: tt.wlogterm},
 		}
 		if !reflect.DeepEqual(msgs, wmsgs) {
 			t.Errorf("#%d: msgs = %+v, want %+v", i, msgs, wmsgs)

diff --git a/testdata/confchange_v2_replace_leader.txt b/testdata/confchange_v2_replace_leader.txt
@@ -282,12 +282,10 @@ stabilize
   CommittedEntries:
   2/5 EntryNormal ""
   Messages:
-  4->1 MsgApp Term:2 Log:2/5 Commit:4
   4->1 MsgApp Term:2 Log:2/5 Commit:5
   4->2 MsgApp Term:2 Log:2/5 Commit:5
   4->3 MsgApp Term:2 Log:2/5 Commit:5
 > 1 receiving messages
-  4->1 MsgApp Term:2 Log:2/5 Commit:4
   4->1 MsgApp Term:2 Log:2/5 Commit:5
 > 2 receiving messages
   4->2 MsgApp Term:2 Log:2/5 Commit:5
@@ -299,7 +297,6 @@ stabilize
   CommittedEntries:
   2/5 EntryNormal ""
   Messages:
-  1->4 MsgAppResp Term:2 Log:0/5 Commit:4
   1->4 MsgAppResp Term:2 Log:0/5 Commit:5
 > 2 handling Ready
   Ready MustSync=false:
@@ -316,7 +313,6 @@ stabilize
   Messages:
   3->4 MsgAppResp Term:2 Log:0/5 Commit:5
 > 4 receiving messages
-  1->4 MsgAppResp Term:2 Log:0/5 Commit:4
   1->4 MsgAppResp Term:2 Log:0/5 Commit:5
   2->4 MsgAppResp Term:2 Log:0/5 Commit:5
   3->4 MsgAppResp Term:2 Log:0/5 Commit:5

diff --git a/testdata/probe_and_replicate.txt b/testdata/probe_and_replicate.txt
@@ -507,18 +507,6 @@ stabilize 1 2
   2->1 MsgAppResp Term:8 Log:0/21 Commit:18
 > 1 receiving messages
   2->1 MsgAppResp Term:8 Log:0/21 Commit:18
-> 1 handling Ready
-  Ready MustSync=false:
-  Messages:
-  1->2 MsgApp Term:8 Log:8/21 Commit:18
-> 2 receiving messages
-  1->2 MsgApp Term:8 Log:8/21 Commit:18
-> 2 handling Ready
-  Ready MustSync=false:
-  Messages:
-  2->1 MsgAppResp Term:8 Log:0/21 Commit:18
-> 1 receiving messages
-  2->1 MsgAppResp Term:8 Log:0/21 Commit:18
 
 stabilize 1 3
 ----
@@ -557,18 +545,6 @@ stabilize 1 3
   3->1 MsgAppResp Term:8 Log:0/21 Commit:18
 > 1 receiving messages
   3->1 MsgAppResp Term:8 Log:0/21 Commit:18
-> 1 handling Ready
-  Ready MustSync=false:
-  Messages:
-  1->3 MsgApp Term:8 Log:8/21 Commit:18
-> 3 receiving messages
-  1->3 MsgApp Term:8 Log:8/21 Commit:18
-> 3 handling Ready
-  Ready MustSync=false:
-  Messages:
-  3->1 MsgAppResp Term:8 Log:0/21 Commit:18
-> 1 receiving messages
-  3->1 MsgAppResp Term:8 Log:0/21 Commit:18
 
 stabilize 1 4
 ----
@@ -644,18 +620,6 @@ stabilize 1 5
   5->1 MsgAppResp Term:8 Log:0/21 Commit:21
 > 1 receiving messages
   5->1 MsgAppResp Term:8 Log:0/21 Commit:21
-> 1 handling Ready
-  Ready MustSync=false:
-  Messages:
-  1->5 MsgApp Term:8 Log:8/21 Commit:21
-> 5 receiving messages
-  1->5 MsgApp Term:8 Log:8/21 Commit:21
-> 5 handling Ready
-  Ready MustSync=false:
-  Messages:
-  5->1 MsgAppResp Term:8 Log:0/21 Commit:21
-> 1 receiving messages
-  5->1 MsgAppResp Term:8 Log:0/21 Commit:21
 
 stabilize 1 6
 ----
@@ -697,18 +661,6 @@ stabilize 1 6
   6->1 MsgAppResp Term:8 Log:0/21 Commit:21
 > 1 receiving messages
   6->1 MsgAppResp Term:8 Log:0/21 Commit:21
-> 1 handling Ready
-  Ready MustSync=false:
-  Messages:
-  1->6 MsgApp Term:8 Log:8/21 Commit:21
-> 6 receiving messages
-  1->6 MsgApp Term:8 Log:8/21 Commit:21
-> 6 handling Ready
-  Ready MustSync=false:
-  Messages:
-  6->1 MsgAppResp Term:8 Log:0/21 Commit:21
-> 1 receiving messages
-  6->1 MsgAppResp Term:8 Log:0/21 Commit:21
 
 stabilize 1 7
 ----
@@ -754,15 +706,3 @@ stabilize 1 7
   7->1 MsgAppResp Term:8 Log:0/21 Commit:21
 > 1 receiving messages
   7->1 MsgAppResp Term:8 Log:0/21 Commit:21
-> 1 handling Ready
-  Ready MustSync=false:
-  Messages:
-  1->7 MsgApp Term:8 Log:8/21 Commit:21
-> 7 receiving messages
-  1->7 MsgApp Term:8 Log:8/21 Commit:21
-> 7 handling Ready
-  Ready MustSync=false:
-  Messages:
-  7->1 MsgAppResp Term:8 Log:0/21 Commit:21
-> 1 receiving messages
-  7->1 MsgAppResp Term:8 Log:0/21 Commit:21