From 95c5fed3cf306774e98010296ca7b71411346067 Mon Sep 17 00:00:00 2001
From: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Date: Thu, 29 Sep 2022 23:53:10 -0400
Subject: [PATCH 1/2] raft: remove IsEmptySnap check from
 raftLog.hasPendingSnapshot

unstable.snapshot is never an empty snapshot. This check made it look
like it could be.

Signed-off-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
---
 raft/log.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/raft/log.go b/raft/log.go
index 118f8d07883..912eb50b078 100644
--- a/raft/log.go
+++ b/raft/log.go
@@ -201,7 +201,7 @@ func (l *raftLog) hasNextCommittedEnts() bool {
 
 // hasPendingSnapshot returns if there is pending snapshot waiting for applying.
 func (l *raftLog) hasPendingSnapshot() bool {
-	return l.unstable.snapshot != nil && !IsEmptySnap(*l.unstable.snapshot)
+	return l.unstable.snapshot != nil
 }
 
 func (l *raftLog) snapshot() (pb.Snapshot, error) {

From 539a8410f40a8b5708ce3f6b44c2885f81094ee4 Mon Sep 17 00:00:00 2001
From: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Date: Tue, 18 Oct 2022 14:19:00 -0400
Subject: [PATCH 2/2] raft: don't apply entries when applying snapshot

This commit removes the ability to apply log entries at the same time as
applying a snapshot. Doing so it possible, but it leads to complex code and
raises questions about what should be applied first. It also raises additional
complexity when we start allowing concurrent, asynchronous log appends and log
application. It's easiest to just disallow this.

Signed-off-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
---
 raft/log.go      | 19 ++++++++++++++-----
 raft/log_test.go | 40 ++++++++++++++++++++++++++++------------
 2 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/raft/log.go b/raft/log.go
index 912eb50b078..6bf9d153e8c 100644
--- a/raft/log.go
+++ b/raft/log.go
@@ -181,9 +181,13 @@ func (l *raftLog) unstableEntries() []pb.Entry {
 // If applied is smaller than the index of snapshot, it returns all committed
 // entries after the index of snapshot.
 func (l *raftLog) nextCommittedEnts() (ents []pb.Entry) {
-	off := max(l.applied+1, l.firstIndex())
-	if l.committed+1 > off {
-		ents, err := l.slice(off, l.committed+1, l.maxNextCommittedEntsSize)
+	if l.hasPendingSnapshot() {
+		// See comment in hasNextCommittedEnts.
+		return nil
+	}
+	if l.committed > l.applied {
+		lo, hi := l.applied+1, l.committed+1 // [lo, hi)
+		ents, err := l.slice(lo, hi, l.maxNextCommittedEntsSize)
 		if err != nil {
 			l.logger.Panicf("unexpected error when getting unapplied entries (%v)", err)
 		}
@@ -195,8 +199,13 @@ func (l *raftLog) nextCommittedEnts() (ents []pb.Entry) {
 // hasNextCommittedEnts returns if there is any available entries for execution.
 // This is a fast check without heavy raftLog.slice() in nextCommittedEnts().
 func (l *raftLog) hasNextCommittedEnts() bool {
-	off := max(l.applied+1, l.firstIndex())
-	return l.committed+1 > off
+	if l.hasPendingSnapshot() {
+		// If we have a snapshot to apply, don't also return any committed
+		// entries. Doing so raises questions about what should be applied
+		// first.
+		return false
+	}
+	return l.committed > l.applied
 }
 
 // hasPendingSnapshot returns if there is pending snapshot waiting for applying.
diff --git a/raft/log_test.go b/raft/log_test.go
index adeaee3ea95..546b1c3039a 100644
--- a/raft/log_test.go
+++ b/raft/log_test.go
@@ -349,13 +349,16 @@ func TestHasNextCommittedEnts(t *testing.T) {
 		{Term: 1, Index: 6},
 	}
 	tests := []struct {
-		applied uint64
-		hasNext bool
+		applied  uint64
+		snap     bool
+		whasNext bool
 	}{
-		{0, true},
-		{3, true},
-		{4, true},
-		{5, false},
+		{applied: 0, snap: false, whasNext: true},
+		{applied: 3, snap: false, whasNext: true},
+		{applied: 4, snap: false, whasNext: true},
+		{applied: 5, snap: false, whasNext: false},
+		// With snapshot.
+		{applied: 3, snap: true, whasNext: false},
 	}
 	for i, tt := range tests {
 		storage := NewMemoryStorage()
@@ -364,10 +367,15 @@ func TestHasNextCommittedEnts(t *testing.T) {
 		raftLog.append(ents...)
 		raftLog.maybeCommit(5, 1)
 		raftLog.appliedTo(tt.applied)
+		if tt.snap {
+			newSnap := snap
+			newSnap.Metadata.Index++
+			raftLog.restore(newSnap)
+		}
 
 		hasNext := raftLog.hasNextCommittedEnts()
-		if hasNext != tt.hasNext {
-			t.Errorf("#%d: hasNext = %v, want %v", i, hasNext, tt.hasNext)
+		if hasNext != tt.whasNext {
+			t.Errorf("#%d: hasNext = %v, want %v", i, hasNext, tt.whasNext)
 		}
 	}
 }
@@ -383,12 +391,15 @@ func TestNextCommittedEnts(t *testing.T) {
 	}
 	tests := []struct {
 		applied uint64
+		snap    bool
 		wents   []pb.Entry
 	}{
-		{0, ents[:2]},
-		{3, ents[:2]},
-		{4, ents[1:2]},
-		{5, nil},
+		{applied: 0, snap: false, wents: ents[:2]},
+		{applied: 3, snap: false, wents: ents[:2]},
+		{applied: 4, snap: false, wents: ents[1:2]},
+		{applied: 5, snap: false, wents: nil},
+		// With snapshot.
+		{applied: 3, snap: true, wents: nil},
 	}
 	for i, tt := range tests {
 		storage := NewMemoryStorage()
@@ -397,6 +408,11 @@ func TestNextCommittedEnts(t *testing.T) {
 		raftLog.append(ents...)
 		raftLog.maybeCommit(5, 1)
 		raftLog.appliedTo(tt.applied)
+		if tt.snap {
+			newSnap := snap
+			newSnap.Metadata.Index++
+			raftLog.restore(newSnap)
+		}
 
 		nents := raftLog.nextCommittedEnts()
 		if !reflect.DeepEqual(nents, tt.wents) {