kvserver: drop RemoveReplica opts parameter

All callers pass in the same opts, so no need in making it configurable.

Epic: none
Release note: none

Author: pav-kv

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_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_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_remove_replica.go

@@ -33,16 +33,11 @@ type RemoveOptions struct {
 // removal decision is passed in. Removal is aborted if the replica ID has
 // advanced to or beyond the NextReplicaID since the removal decision was made.
 //
-// If opts.DestroyReplica is false, replica.destroyRaftMuLocked is not called.
-//
 // The passed replica must be initialized.
 func (s *Store) RemoveReplica(
-	ctx context.Context,
-	rep *Replica,
-	nextReplicaID roachpb.ReplicaID,
-	reason redact.SafeString,
-	opts RemoveOptions,
+	ctx context.Context, rep *Replica, nextReplicaID roachpb.ReplicaID, reason redact.SafeString,
 ) error {
+	opts := RemoveOptions{DestroyData: true}
 	rep.raftMu.Lock()
 	defer rep.raftMu.Unlock()
 	if opts.InsertPlaceholder {
@@ -77,6 +72,8 @@ func (s *Store) removeReplicaRaftMuLocked(
 // removeInitializedReplicaRaftMuLocked is the implementation of RemoveReplica,
 // which is sometimes called directly when the necessary lock is already held.
 // It requires that Replica.raftMu is held and that s.mu is not held.
+//
+// If opts.DestroyData is false, replica.destroyRaftMuLocked is not called.
 func (s *Store) removeInitializedReplicaRaftMuLocked(
 	ctx context.Context,
 	rep *Replica,

pkg/kv/kvserver/store_test.go

@@ -71,6 +71,7 @@ import (
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/redact"
 	"github.com/kr/pretty"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/exp/slices"
 	"golang.org/x/sync/errgroup"
@@ -620,11 +621,7 @@ func TestStoreAddRemoveRanges(t *testing.T) {
 		t.Error(err)
 	}
 	// Remove range 1.
-	if err := store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-		DestroyData: true,
-	}); err != nil {
-		t.Error(err)
-	}
+	assert.NoError(t, store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name())))
 	// Create a new range (id=2).
 	repl2 := createReplica(store, 2, roachpb.RKey("a"), roachpb.RKey("b"))
 	if err := store.AddReplica(repl2); err != nil {
@@ -636,11 +633,7 @@ func TestStoreAddRemoveRanges(t *testing.T) {
 		t.Fatal("expected error re-adding same range")
 	}
 	// Try to remove range 1 again.
-	if err := store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-		DestroyData: true,
-	}); err != nil {
-		t.Fatalf("didn't expect error re-removing same range: %v", err)
-	}
+	require.NoError(t, store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name())))
 	// Try to add a range with previously-used (but now removed) ID.
 	repl2Dup := createReplica(store, 1, roachpb.RKey("a"), roachpb.RKey("b"))
 	if err := store.AddReplica(repl2Dup); err == nil {
@@ -742,26 +735,29 @@ func TestStoreRemoveReplicaDestroy(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Can't remove Replica with DestroyData false because this requires the destroyStatus
-	// to already have been set by the caller (but we didn't).
-	require.ErrorContains(t, store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-		DestroyData: false,
-	}), `replica not marked as destroyed`)
+	rmWithoutData := func() error {
+		repl1.raftMu.Lock()
+		defer repl1.raftMu.Unlock()
+		_, err := store.removeInitializedReplicaRaftMuLocked(
+			ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name()),
+			RemoveOptions{DestroyData: false},
+		)
+		return err
+	}
+	// Can't remove Replica with DestroyData false because this requires the
+	// destroyStatus to already have been set by the caller (but we didn't).
+	require.ErrorContains(t, rmWithoutData(), `replica not marked as destroyed`)
 
 	// Remove the Replica twice, as this should be idempotent.
 	// NB: we rely on this idempotency today (as @tbg found out when he accidentally
 	// removed it).
 	for i := 0; i < 2; i++ {
-		require.NoError(t, store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-			DestroyData: true,
-		}), "%d", i)
+		require.NoError(t, store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name())), "%d", i)
 	}
 
-	// However, if we have DestroyData=false, caller is expected to be the unique first "destroyer"
-	// of the Replica.
-	require.ErrorContains(t, store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-		DestroyData: false,
-	}), `does not exist`)
+	// However, if we have DestroyData=false, caller is expected to be the unique
+	// first "destroyer" of the Replica.
+	require.ErrorContains(t, rmWithoutData(), `does not exist`)
 
 	// Verify that removal of a replica marks it as destroyed so that future raft
 	// commands on the Replica will silently be dropped.
@@ -802,11 +798,7 @@ func TestStoreReplicaVisitor(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	if err := store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-		DestroyData: true,
-	}); err != nil {
-		t.Error(err)
-	}
+	assert.NoError(t, store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name())))
 
 	// Add 10 new ranges.
 	const newCount = 10
@@ -883,14 +875,8 @@ func TestMarkReplicaInitialized(t *testing.T) {
 
 	// Clobber the existing range so we can test overlaps that aren't KeyMin or KeyMax.
 	repl1, err := store.GetReplica(1)
-	if err != nil {
-		t.Error(err)
-	}
-	if err := store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-		DestroyData: true,
-	}); err != nil {
-		t.Error(err)
-	}
+	assert.NoError(t, err)
+	assert.NoError(t, store.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name())))
 
 	repl := createReplica(store, roachpb.RangeID(2), roachpb.RKey("a"), roachpb.RKey("c"))
 	if err := store.AddReplica(repl); err != nil {
@@ -2754,14 +2740,8 @@ func TestMaybeRemove(t *testing.T) {
 	store.WaitForInit()
 
 	repl, err := store.GetReplica(1)
-	if err != nil {
-		t.Error(err)
-	}
-	if err := store.RemoveReplica(ctx, repl, repl.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-		DestroyData: true,
-	}); err != nil {
-		t.Error(err)
-	}
+	assert.NoError(t, err)
+	assert.NoError(t, store.RemoveReplica(ctx, repl, repl.Desc().NextReplicaID, redact.SafeString(t.Name())))
 	// MaybeRemove is called.
 	removedRng := <-fq.maybeRemovedRngs
 	if removedRng != repl.RangeID {
@@ -2873,14 +2853,8 @@ func TestStoreRangePlaceholders(t *testing.T) {
 
 	// Clobber the existing range so we can test non-overlapping placeholders.
 	repl1, err := s.GetReplica(1)
-	if err != nil {
-		t.Error(err)
-	}
-	if err := s.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-		DestroyData: true,
-	}); err != nil {
-		t.Error(err)
-	}
+	assert.NoError(t, err)
+	assert.NoError(t, s.RemoveReplica(ctx, repl1, repl1.Desc().NextReplicaID, redact.SafeString(t.Name())))
 
 	repID := roachpb.RangeID(2)
 	rep := createReplica(s, repID, roachpb.RKeyMin, roachpb.RKey("c"))
@@ -3012,9 +2986,7 @@ func TestStoreRemovePlaceholderOnRaftIgnored(t *testing.T) {
 	repl1, err := s.GetReplica(1)
 	desc := repl1.Desc()
 	require.NoError(t, err)
-	require.NoError(t, s.RemoveReplica(ctx, repl1, desc.NextReplicaID, redact.SafeString(t.Name()), RemoveOptions{
-		DestroyData: true,
-	}))
+	require.NoError(t, s.RemoveReplica(ctx, repl1, desc.NextReplicaID, redact.SafeString(t.Name())))
 
 	// Wrap the snapshot in a minimal header. The request will be dropped because
 	// replica 2 is not in the ConfState.