Merge #155239

155239: kvserver: fixups around replica GC queue r=stevendanna a=pav-kv

Epic: none
Release note: none

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

Author: craig[bot]

pkg/kv/kvserver/replica_gc_queue.go

@@ -306,16 +306,10 @@ func (rgcq *replicaGCQueue) process(
 		log.VEventf(ctx, 1, "destroying local data")
 
 		nextReplicaID := replyDesc.NextReplicaID
-		// Note that this seems racy - we didn't hold any locks between reading
-		// the range descriptor above and deciding to remove the replica - but
-		// we pass in the NextReplicaID to detect situations in which the
-		// replica became "non-gc'able" in the meantime by checking (with raftMu
-		// held throughout) whether the replicaID is still smaller than the
-		// NextReplicaID. Given non-zero replica IDs don't change, this is only
-		// 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"); err != nil {
+		// NB: since we didn't hold any locks between reading the range descriptor
+		// above and deciding to remove the replica, it could have been removed
+		// concurrently. RemoveReplica gracefully returns nil in this case.
+		if err := repl.store.RemoveReplica(ctx, repl, nextReplicaID, "replica 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)
@@ -358,7 +352,7 @@ func (rgcq *replicaGCQueue) process(
 		// 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",
+			ctx, repl, mergedTombstoneReplicaID, "dangling subsume via replica GC queue",
 		); err != nil {
 			return false, err
 		}

pkg/kv/kvserver/store_remove_replica.go

@@ -27,11 +27,11 @@ type RemoveOptions struct {
 }
 
 // RemoveReplica removes the replica from the store's replica map and from the
-// sorted replicasByKey btree.
+// sorted replicasByKey btree. If the replica is already removed, returns nil.
 //
-// The NextReplicaID from the replica descriptor that was used to make the
-// 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.
+// The passed in NextReplicaID is taken from the replica descriptor used to make
+// the removal decision. It is written into the RangeTombstone and prevents
+// historical replicas (including this one) from appearing on this store again.
 //
 // The passed replica must be initialized.
 func (s *Store) RemoveReplica(