Merge #150318

150318: kvserver: drop opts in replica removal r=tbg a=pav-kv

All callers pass in the same opts, so no need in making it configurable. After this PR, `removeInitializedReplicaRaftMuLocked` is the only remaining call accepting `RemoveOptions`, which makes the scope/purpose of these options clearer.

This also does a couple of drive-by clean-ups in replica destruction stack.

Epic: CRDB-49111

Co-authored-by: Pavel Kalinnikov <[email protected]>

Author: craig[bot]

pkg/kv/kvserver/client_raft_test.go

@@ -3087,9 +3087,9 @@ func TestReplicaRemovalCampaign(t *testing.T) {
 
 			if td.remove {
 				// Simulate second replica being transferred by removing it.
-				if err := store0.RemoveReplica(ctx, replica2, replica2.Desc().NextReplicaID, redact.SafeString(t.Name()), kvserver.RemoveOptions{
-					DestroyData: true,
-				}); err != nil {
+				if err := store0.RemoveReplica(
+					ctx, replica2, replica2.Desc().NextReplicaID, redact.SafeString(t.Name()),
+				); err != nil {
 					t.Fatal(err)
 				}
 			}

pkg/kv/kvserver/mvcc_gc_queue_test.go

@@ -1511,7 +1511,7 @@ func TestMVCCGCQueueGroupsRangeDeletions(t *testing.T) {
 	store := createTestStoreWithConfig(ctx, t, stopper, testStoreOpts{createSystemRanges: false}, &cfg)
 	r, err := store.GetReplica(roachpb.RangeID(1))
 	require.NoError(t, err)
-	require.NoError(t, store.RemoveReplica(ctx, r, r.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{DestroyData: true}))
+	require.NoError(t, store.RemoveReplica(ctx, r, r.Desc().NextReplicaID, redact.SafeString(t.Name())))
 	// Add replica without hint.
 	r1 := createReplica(store, roachpb.RangeID(100), key("a"), key("b"))
 	require.NoError(t, store.AddReplica(r1))

pkg/kv/kvserver/queue_test.go

@@ -123,11 +123,8 @@ func createReplicas(t *testing.T, tc *testContext, num int) []*Replica {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if err := tc.store.RemoveReplica(context.Background(), repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-		DestroyData: true,
-	}); err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, tc.store.RemoveReplica(context.Background(),
+		repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name())))
 
 	repls := make([]*Replica, num)
 	for i := 0; i < num; i++ {
@@ -708,7 +705,7 @@ func TestAcceptsUnsplitRanges(t *testing.T) {
 	// Remove replica for range 1 since it encompasses the entire keyspace.
 	repl1, err := s.GetReplica(1)
 	require.NoError(t, err)
-	require.NoError(t, s.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{DestroyData: true}))
+	require.NoError(t, s.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name())))
 
 	// This range can never be split due to zone configs boundaries.
 	neverSplits := createReplica(s, 2, roachpb.RKeyMin, maxWontSplitAddr)
@@ -910,11 +907,8 @@ func TestBaseQueuePurgatory(t *testing.T) {
 	// the replica set. The number of processed replicas will be 2 less.
 	const rmReplCount = 2
 	repls[0].replicaID = 2
-	if err := tc.store.RemoveReplica(ctx, repls[1], repls[1].Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-		DestroyData: true,
-	}); err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, tc.store.RemoveReplica(ctx,
+		repls[1], repls[1].Desc().NextReplicaID, redact.SafeString(t.Name())))
 
 	// Remove error and reprocess.
 	testQueue.err = nil

pkg/kv/kvserver/replica_application_result.go

@@ -698,7 +698,7 @@ func (r *Replica) handleChangeReplicasResult(
 	// NB: postDestroyRaftMuLocked requires that the batch which removed the data
 	// be durably synced to disk, which we have.
 	// See replicaAppBatch.ApplyToStateMachine().
-	if err := r.postDestroyRaftMuLocked(ctx, r.GetMVCCStats()); err != nil {
+	if err := r.postDestroyRaftMuLocked(ctx); err != nil {
 		log.Fatalf(ctx, "failed to run Replica postDestroy: %v", err)
 	}
 

pkg/kv/kvserver/replica_destroy.go

@@ -14,7 +14,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/apply"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
-	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 )
@@ -65,20 +64,12 @@ func (s destroyStatus) Removed() bool {
 // don't know the current replica ID.
 const mergedTombstoneReplicaID roachpb.ReplicaID = math.MaxInt32
 
-func (r *Replica) postDestroyRaftMuLocked(ctx context.Context, ms enginepb.MVCCStats) error {
-	// TODO(tschottdorf): at node startup, we should remove all on-disk
-	// directories belonging to replicas which aren't present. A crash before a
-	// call to postDestroyRaftMuLocked will currently leave the files around
-	// forever.
-	//
-	// TODO(tbg): coming back in 2021, the above should be outdated. The ReplicaID
-	// is set on creation and never changes over the lifetime of a Replica. Also,
-	// the replica is always contained in its descriptor. So this code below should
-	// be removable.
-	//
-	// TODO(pavelkalinnikov): coming back in 2023, the above may still happen if:
-	// (1) state machine syncs, (2) OS crashes before (3) sideloaded was able to
-	// sync the files removal. The files should be cleaned up on restart.
+// postDestroyRaftMuLocked is called after the replica destruction is durably
+// written to Pebble.
+func (r *Replica) postDestroyRaftMuLocked(ctx context.Context) error {
+	// TODO(#136416): at node startup, we should remove all on-disk directories
+	// belonging to replicas which aren't present. A crash before a call to
+	// postDestroyRaftMuLocked will currently leave the files around forever.
 	if err := r.logStorage.ls.Sideload.Clear(ctx); err != nil {
 		return err
 	}
@@ -135,7 +126,7 @@ func (r *Replica) destroyRaftMuLocked(ctx context.Context, nextReplicaID roachpb
 	}
 	commitTime := timeutil.Now()
 
-	if err := r.postDestroyRaftMuLocked(ctx, ms); err != nil {
+	if err := r.postDestroyRaftMuLocked(ctx); err != nil {
 		return err
 	}
 	if r.IsInitialized() {

pkg/kv/kvserver/replica_gc_queue.go

@@ -315,9 +315,7 @@ func (rgcq *replicaGCQueue) process(
 		// possible if we currently think we're processing a pre-emptive snapshot
 		// but discover in RemoveReplica that this range has since been added and
 		// knows that.
-		if err := repl.store.RemoveReplica(ctx, repl, nextReplicaID, "MVCC GC queue", RemoveOptions{
-			DestroyData: true,
-		}); err != nil {
+		if err := repl.store.RemoveReplica(ctx, repl, nextReplicaID, "MVCC GC queue"); err != nil {
 			// Should never get an error from RemoveReplica.
 			const format = "error during replicaGC: %v"
 			logcrash.ReportOrPanic(ctx, &repl.store.ClusterSettings().SV, format, err)
@@ -359,9 +357,9 @@ func (rgcq *replicaGCQueue) process(
 		// A tombstone is written with a value of mergedTombstoneReplicaID because
 		// we know the range to have been merged. See the Merge case of
 		// runPreApplyTriggers() for details.
-		if err := repl.store.RemoveReplica(ctx, repl, mergedTombstoneReplicaID, "dangling subsume via MVCC GC queue", RemoveOptions{
-			DestroyData: true,
-		}); err != nil {
+		if err := repl.store.RemoveReplica(
+			ctx, repl, mergedTombstoneReplicaID, "dangling subsume via MVCC GC queue",
+		); err != nil {
 			return false, err
 		}
 	}

pkg/kv/kvserver/replica_raftstorage.go

@@ -815,7 +815,7 @@ func (r *Replica) clearSubsumedReplicaInMemoryData(
 		phs = append(phs, ph)
 		// We removed sr's data when we committed the batch. Finish subsumption by
 		// updating the in-memory bookkeping.
-		if err := sr.postDestroyRaftMuLocked(ctx, sr.GetMVCCStats()); err != nil {
+		if err := sr.postDestroyRaftMuLocked(ctx); err != nil {
 			return nil, err
 		}
 	}

pkg/kv/kvserver/replica_test.go

@@ -9226,7 +9226,7 @@ func TestCancelPendingCommands(t *testing.T) {
 		t.Fatalf("command finished earlier than expected with error %v", pErr)
 	default:
 	}
-	require.NoError(t, tc.store.RemoveReplica(ctx, tc.repl, tc.repl.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{DestroyData: true}))
+	require.NoError(t, tc.store.RemoveReplica(ctx, tc.repl, tc.repl.Desc().NextReplicaID, redact.SafeString(t.Name())))
 	pErr := <-errChan
 	if _, ok := pErr.GetDetail().(*kvpb.AmbiguousResultError); !ok {
 		t.Errorf("expected AmbiguousResultError, got %v", pErr)

pkg/kv/kvserver/store_create_replica.go

@@ -116,9 +116,9 @@ func (s *Store) tryGetReplica(
 		}
 
 		repl.mu.RUnlock()
-		if err := s.removeReplicaRaftMuLocked(ctx, repl, id.ReplicaID, "superseded by newer Replica", RemoveOptions{
-			DestroyData: true,
-		}); err != nil {
+		if err := s.removeReplicaRaftMuLocked(
+			ctx, repl, id.ReplicaID, "superseded by newer Replica",
+		); err != nil {
 			log.Fatalf(ctx, "failed to remove replica: %v", err)
 		}
 		repl.raftMu.Unlock()

pkg/kv/kvserver/store_merge.go

@@ -138,7 +138,7 @@ func (s *Store) MergeRange(
 		return errors.Wrap(err, "cannot remove range")
 	}
 
-	if err := rightRepl.postDestroyRaftMuLocked(ctx, rightRepl.GetMVCCStats()); err != nil {
+	if err := rightRepl.postDestroyRaftMuLocked(ctx); err != nil {
 		return err
 	}