Merge cockroachdb#126337

126337: raft: rm redundant return value from maybeAppend r=miraradeva a=pav-kv

The caller knows the last appended index and compute it themselves if the append succeeds.

Epic: none
Release note: none

Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

pkg/raft/log.go

@@ -103,12 +103,15 @@ func (l *raftLog) String() string {
 		l.committed, l.applied, l.applying, l.unstable.offset, l.unstable.offsetInProgress, len(l.unstable.entries))
 }
 
-// maybeAppend returns (0, false) if the entries cannot be appended. Otherwise,
-// it returns (last index of new entries, true).
-func (l *raftLog) maybeAppend(a logSlice) (lastnewi uint64, ok bool) {
+// maybeAppend appends the given log slice to the log. Returns false if the
+// slice can not be appended (because it is out of bounds, or a.prev does not
+// match the log).
+//
+// TODO(pav-kv): merge maybeAppend and append into one method.
+func (l *raftLog) maybeAppend(a logSlice) bool {
 	match, ok := l.findConflict(a)
 	if !ok {
-		return 0, false
+		return false
 	}
 
 	// Fast-forward to the first mismatching or missing entry.
@@ -119,7 +122,7 @@ func (l *raftLog) maybeAppend(a logSlice) (lastnewi uint64, ok bool) {
 	// TODO(pav-kv): pass the logSlice down the stack, for safety checks and
 	// bookkeeping in the unstable structure.
 	l.append(a.entries...)
-	return a.lastIndex(), true
+	return true
 }
 
 func (l *raftLog) append(ents ...pb.Entry) {

pkg/raft/log_test.go

@@ -226,7 +226,6 @@ func TestLogMaybeAppend(t *testing.T) {
 		committed uint64
 		ents      []pb.Entry
 
-		wlasti  uint64
 		wappend bool
 		wcommit uint64
 		wpanic  bool
@@ -235,75 +234,75 @@ func TestLogMaybeAppend(t *testing.T) {
 		{
 			entryID{term: lastterm - 1, index: lastindex}, lastindex,
 			index(lastindex + 1).terms(4),
-			0, false, commit, false,
+			false, commit, false,
 		},
 		// not match: index out of bound
 		{
 			entryID{term: lastterm, index: lastindex + 1}, lastindex,
 			index(lastindex + 2).terms(4),
-			0, false, commit, false,
+			false, commit, false,
 		},
 		// match with the last existing entry
 		{
 			entryID{term: lastterm, index: lastindex}, lastindex, nil,
-			lastindex, true, lastindex, false,
+			true, lastindex, false,
 		},
 		{
 			entryID{term: lastterm, index: lastindex}, lastindex + 1, nil,
-			lastindex, true, lastindex, false, // do not increase commit higher than lastnewi
+			true, lastindex, false, // do not increase commit higher than lastnewi
 		},
 		{
 			entryID{term: lastterm, index: lastindex}, lastindex - 1, nil,
-			lastindex, true, lastindex - 1, false, // commit up to the commit in the message
+			true, lastindex - 1, false, // commit up to the commit in the message
 		},
 		{
 			entryID{term: lastterm, index: lastindex}, 0, nil,
-			lastindex, true, commit, false, // commit do not decrease
+			true, commit, false, // commit do not decrease
 		},
 		{
 			entryID{}, lastindex, nil,
-			0, true, commit, false, // commit do not decrease
+			true, commit, false, // commit do not decrease
 		},
 		{
 			entryID{term: lastterm, index: lastindex}, lastindex,
 			index(lastindex + 1).terms(4),
-			lastindex + 1, true, lastindex, false,
+			true, lastindex, false,
 		},
 		{
 			entryID{term: lastterm, index: lastindex}, lastindex + 1,
 			index(lastindex + 1).terms(4),
-			lastindex + 1, true, lastindex + 1, false,
+			true, lastindex + 1, false,
 		},
 		{
 			entryID{term: lastterm, index: lastindex}, lastindex + 2,
 			index(lastindex + 1).terms(4),
-			lastindex + 1, true, lastindex + 1, false, // do not increase commit higher than lastnewi
+			true, lastindex + 1, false, // do not increase commit higher than lastnewi
 		},
 		{
 			entryID{term: lastterm, index: lastindex}, lastindex + 2,
 			index(lastindex+1).terms(4, 4),
-			lastindex + 2, true, lastindex + 2, false,
+			true, lastindex + 2, false,
 		},
 		// match with the entry in the middle
 		{
 			entryID{term: lastterm - 1, index: lastindex - 1}, lastindex,
 			index(lastindex).terms(4),
-			lastindex, true, lastindex, false,
+			true, lastindex, false,
 		},
 		{
 			entryID{term: lastterm - 2, index: lastindex - 2}, lastindex,
 			index(lastindex - 1).terms(4),
-			lastindex - 1, true, lastindex - 1, false,
+			true, lastindex - 1, false,
 		},
 		{
 			entryID{term: lastterm - 3, index: lastindex - 3}, lastindex,
 			index(lastindex - 2).terms(4),
-			lastindex - 2, true, lastindex - 2, true, // conflict with existing committed entry
+			true, lastindex - 2, true, // conflict with existing committed entry
 		},
 		{
 			entryID{term: lastterm - 2, index: lastindex - 2}, lastindex,
 			index(lastindex-1).terms(4, 4),
-			lastindex, true, lastindex, false,
+			true, lastindex, false,
 		},
 	}
 
@@ -328,11 +327,10 @@ func TestLogMaybeAppend(t *testing.T) {
 					require.True(t, tt.wpanic)
 				}
 			}()
-			glasti, gappend := raftLog.maybeAppend(app)
-			require.Equal(t, tt.wlasti, glasti)
+			gappend := raftLog.maybeAppend(app)
 			require.Equal(t, tt.wappend, gappend)
 			if gappend {
-				raftLog.commitTo(min(glasti, tt.committed))
+				raftLog.commitTo(min(app.lastIndex(), tt.committed))
 			}
 			require.Equal(t, tt.wcommit, raftLog.committed)
 			if gappend && len(tt.ents) != 0 {

pkg/raft/raft.go

@@ -1681,14 +1681,15 @@ func (r *raft) handleAppendEntries(m pb.Message) {
 		r.send(pb.Message{To: m.From, Type: pb.MsgAppResp, Index: r.raftLog.committed})
 		return
 	}
-	if mlastIndex, ok := r.raftLog.maybeAppend(a); ok {
+	if r.raftLog.maybeAppend(a) {
 		r.accTerm = m.Term // our log is now consistent with the m.Term leader
 		// TODO(pav-kv): make it possible to commit even if the append did not
 		// succeed or is stale. If r.accTerm >= m.Term, then our log contains all
 		// committed entries at m.Term (by raft invariants), so it is safe to bump
 		// the commit index even if the MsgApp is stale.
-		r.raftLog.commitTo(min(m.Commit, mlastIndex))
-		r.send(pb.Message{To: m.From, Type: pb.MsgAppResp, Index: mlastIndex})
+		lastIndex := a.lastIndex()
+		r.raftLog.commitTo(min(m.Commit, lastIndex))
+		r.send(pb.Message{To: m.From, Type: pb.MsgAppResp, Index: lastIndex})
 		return
 	}
 	r.logger.Debugf("%x [logterm: %d, index: %d] rejected MsgApp [logterm: %d, index: %d] from %x",