Merge #145328

145328: kvserver: extract unreplicated SST writing in applySnapshot r=tbg a=tbg

The engine (pebble) interactions in applySnapshot are complex and
difficult to test in isolation. By extracting these responsibilities
into a standalone function, we make it easier to unit test and further
refactor the snapshot application process.

To this end, this PR introduces a new prepareSnapshot function in
snapshot_apply_prepare.go, and moves the construction of the SSTs
(and generally all engine interactions prior to the commit) there.

Epic: CRDB-46488

Release note: None

Co-authored-by: Tobias Grieger <[email protected]>

Author: craig[bot]

pkg/kv/kvserver/BUILD.bazel

@@ -85,6 +85,7 @@ go_library(
         "replicate_queue.go",
         "scanner.go",
         "scheduler.go",
+        "snapshot_apply_prepare.go",
         "snapshot_settings.go",
         "split_delay_helper.go",
         "split_queue.go",

pkg/kv/kvserver/client_replica_test.go

@@ -3069,7 +3069,7 @@ func TestClearRange(t *testing.T) {
 // significantly more rare. This test uses a knob to disable the new protection
 // so that it can create the scenario where a replica learns that it holds the
 // lease through a snapshot. We'll want to keep the test and the corresponding
-// logic in applySnapshot around until we can eliminate the scenario entirely.
+// logic in applySnapshotRaftMuLocked around until we can eliminate the scenario entirely.
 // See the commentary in github.com/cockroachdb/cockroach/issues/81561 about
 // sending Raft logs in Raft snapshots for a discussion about why this may not
 // be worth eliminating.

pkg/kv/kvserver/replica_app_batch.go

@@ -449,7 +449,7 @@ func (b *replicaAppBatch) runPostAddTriggersReplicaOnly(
 
 		// Delete all of the Replica's data. We're going to delete the hard state too.
 		// We've set the replica's in-mem status to reflect the pending destruction
-		// above, and preDestroyRaftMuLocked will also add a range tombstone to the
+		// above, and DestroyReplica will also add a range tombstone to the
 		// batch, so that when we commit it, the removal is finalized.
 		if err := kvstorage.DestroyReplica(ctx, b.r.RangeID, b.batch, b.batch, change.NextReplicaID(), kvstorage.ClearRangeDataOptions{
 			ClearReplicatedBySpan:      span,

pkg/kv/kvserver/replica_raft.go

@@ -1188,7 +1188,7 @@ func (r *Replica) handleRaftReadyRaftMuLocked(
 			defer releaseMergeLock()
 
 			stats.tSnapBegin = crtime.NowMono()
-			if err := r.applySnapshot(ctx, inSnap, snap, app.HardState, subsumedRepls); err != nil {
+			if err := r.applySnapshotRaftMuLocked(ctx, inSnap, snap, app.HardState, subsumedRepls); err != nil {
 				return stats, errors.Wrap(err, "while applying snapshot")
 			}
 			for _, msg := range app.Responses {
@@ -1206,7 +1206,7 @@ func (r *Replica) handleRaftReadyRaftMuLocked(
 			stats.tSnapEnd = crtime.NowMono()
 			stats.snap.applied = true
 
-			// The raft log state was updated in applySnapshot, but we also want to
+			// The raft log state was updated in applySnapshotRaftMuLocked, but we also want to
 			// reflect these changes in the state variable here.
 			// TODO(pav-kv): this is unnecessary. We only do it because there is an
 			// unconditional storing of this state below. Avoid doing it twice.

pkg/kv/kvserver/replica_raftlog.go

@@ -195,7 +195,7 @@ func (r *replicaLogStorage) LogSnapshot() raft.LogStorageSnapshot {
 //
 // This would require auditing and integrating with the write paths. Today, this
 // type implements only reads, and writes are in various places like the
-// logstore.LogStore type, or applySnapshot.
+// logstore.LogStore type, or applySnapshotRaftMuLocked.
 type replicaRaftMuLogSnap replicaLogStorage
 
 // Entries implements the raft.LogStorageSnapshot interface.