kvstorage: simplify DestroyReplica

Now that the replica subsumption is factored out into its own call, all
the remaining uses of DestroyReplica remove the entirety of the replica.
We thus don't need to pass the ClearRangeDataOptions to DestroyReplica
any longer. The options become an internal implementation detail of the
kvstorage package.

Epic: none
Release note: none

Author: pav-kv

pkg/kv/kvserver/kvstorage/destroy.go

@@ -129,20 +129,29 @@ type DestroyReplicaInfo struct {
 	Keys roachpb.RSpan
 }
 
-// DestroyReplica destroys all or a part of the Replica's state, installing a
-// RangeTombstone in its place. Due to merges, splits, etc, there is a need to
-// control which part of the state this method actually gets to remove, which is
-// done via the provided options[^1]; the caller is always responsible for
-// managing the remaining disk state accordingly.
-//
-// [^1] e.g., on a merge, the user data moves to the subsuming replica and must
-// not be cleared.
+// DestroyReplica destroys the entirety of the replica's state in storage, and
+// installs a RangeTombstone in its place. It handles both uninitialized and
+// initialized replicas uniformly.
 func DestroyReplica(
 	ctx context.Context,
 	reader storage.Reader,
 	writer storage.Writer,
 	info DestroyReplicaInfo,
 	next roachpb.ReplicaID,
+) error {
+	return destroyReplicaImpl(ctx, reader, writer, info, next, ClearRangeDataOptions{
+		ClearReplicatedByRangeID:   true,
+		ClearUnreplicatedByRangeID: true,
+		ClearReplicatedBySpan:      info.Keys,
+	})
+}
+
+func destroyReplicaImpl(
+	ctx context.Context,
+	reader storage.Reader,
+	writer storage.Writer,
+	info DestroyReplicaInfo,
+	next roachpb.ReplicaID,
 	opts ClearRangeDataOptions,
 ) error {
 	if next <= info.ReplicaID {
@@ -205,5 +214,5 @@ func SubsumeReplica(
 	return rditer.SelectOpts{
 		ReplicatedByRangeID:   opts.ClearReplicatedByRangeID,
 		UnreplicatedByRangeID: opts.ClearUnreplicatedByRangeID,
-	}, DestroyReplica(ctx, reader, writer, info, MergedTombstoneReplicaID, opts)
+	}, destroyReplicaImpl(ctx, reader, writer, info, MergedTombstoneReplicaID, opts)
 }

pkg/kv/kvserver/kvstorage/destroy_test.go

@@ -69,13 +69,7 @@ func TestDestroyReplica(t *testing.T) {
 	})
 	mutate("destroy", func(rw storage.ReadWriter) {
 		require.NoError(t, DestroyReplica(
-			ctx, rw, rw,
-			DestroyReplicaInfo{FullReplicaID: r.id, Keys: r.keys}, r.id.ReplicaID+1,
-			ClearRangeDataOptions{
-				ClearUnreplicatedByRangeID: true,
-				ClearReplicatedByRangeID:   true,
-				ClearReplicatedBySpan:      r.keys,
-			},
+			ctx, rw, rw, DestroyReplicaInfo{FullReplicaID: r.id, Keys: r.keys}, r.id.ReplicaID+1,
 		))
 	})
 

pkg/kv/kvserver/replica_app_batch.go

@@ -440,7 +440,6 @@ func (b *replicaAppBatch) runPostAddTriggersReplicaOnly(
 		b.r.shMu.destroyStatus.Set(
 			kvpb.NewRangeNotFoundError(b.r.RangeID, b.r.store.StoreID()),
 			destroyReasonRemoved)
-		span := b.r.descRLocked().RSpan()
 		b.r.mu.Unlock()
 		b.r.readOnlyCmdMu.Unlock()
 		b.changeRemovesReplica = true
@@ -450,14 +449,7 @@ func (b *replicaAppBatch) runPostAddTriggersReplicaOnly(
 		// 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.batch, b.batch,
-			b.r.destroyInfoRaftMuLocked(),
-			change.NextReplicaID(),
-			kvstorage.ClearRangeDataOptions{
-				ClearReplicatedBySpan:      span,
-				ClearReplicatedByRangeID:   true,
-				ClearUnreplicatedByRangeID: true,
-			},
+			ctx, b.batch, b.batch, b.r.destroyInfoRaftMuLocked(), change.NextReplicaID(),
 		); err != nil {
 			return errors.Wrapf(err, "unable to destroy replica before removal")
 		}

pkg/kv/kvserver/replica_destroy.go

@@ -88,23 +88,10 @@ func (r *Replica) destroyRaftMuLocked(ctx context.Context, nextReplicaID roachpb
 	ms := r.GetMVCCStats()
 	batch := r.store.TODOEngine().NewWriteBatch()
 	defer batch.Close()
-	desc := r.Desc()
-	inited := desc.IsInitialized()
-
-	opts := kvstorage.ClearRangeDataOptions{
-		ClearReplicatedBySpan: desc.RSpan(), // zero if !inited
-		// TODO(tbg): if it's uninitialized, we might as well clear
-		// the replicated state because there isn't any. This seems
-		// like it would be simpler, but needs a code audit to ensure
-		// callers don't call this in in-between states where the above
-		// assumption doesn't hold.
-		ClearReplicatedByRangeID:   inited,
-		ClearUnreplicatedByRangeID: true,
-	}
+
 	// TODO(sep-raft-log): need both engines separately here.
 	if err := kvstorage.DestroyReplica(
-		ctx, r.store.TODOEngine(), batch,
-		r.destroyInfoRaftMuLocked(), nextReplicaID, opts,
+		ctx, r.store.TODOEngine(), batch, r.destroyInfoRaftMuLocked(), nextReplicaID,
 	); err != nil {
 		return err
 	}