kvstorage: add testing hook forcing ClearRange

Epic: none
Release note: none

Author: pav-kv

pkg/kv/kvserver/client_merge_test.go

@@ -3815,6 +3815,10 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
+	// The test expects replica destruction paths to use ranged clears rather than
+	// point clears.
+	defer kvstorage.TestingForceClearRange()()
+
 	testutils.RunTrueAndFalse(t, "rebalanceRHSAway", func(t *testing.T, rebalanceRHSAway bool) {
 		// We will be testing the SSTs written on store2's engine.
 		var receivingEng, sendingEng storage.Engine
@@ -3954,22 +3958,24 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 				sst := storage.MakeIngestionSSTWriter(ctx, st, sstFile)
 				defer sst.Close()
 				{
-					// The snapshot code will use ClearRangeWithHeuristic with a
-					// threshold of 1 to clear the range, but this will truncate
-					// the range tombstone to the first key. In this case, the
-					// first key is RangeGCThresholdKey, which doesn't yet exist
-					// in the engine, so we write the Pebble range tombstone
-					// manually.
+					// The snapshot code uses ClearRangeWithHeuristic with a threshold of
+					// 1 to clear the range, but it will truncate the range tombstone to
+					// the first key. In this case, the first key is RangeGCThresholdKey,
+					// which doesn't yet exist in the engine, so we set it manually.
 					sl := rditer.Select(rangeID, rditer.SelectOpts{
 						ReplicatedByRangeID: true,
 					})
 					require.Len(t, sl, 1)
 					s := sl[0]
 					require.NoError(t, sst.ClearRawRange(keys.RangeGCThresholdKey(rangeID), s.EndKey, true, false))
 				}
+				require.NoError(t, kvstorage.MakeStateLoader(rangeID).SetRangeTombstone(
+					context.Background(), &sst,
+					kvserverpb.RangeTombstone{NextReplicaID: math.MaxInt32},
+				))
 				{
-					// Ditto for the unreplicated version, where the first key
-					// happens to be the HardState.
+					// Ditto for the unreplicated version, where the first key happens to
+					// be the HardState.
 					sl := rditer.Select(rangeID, rditer.SelectOpts{
 						UnreplicatedByRangeID: true,
 					})
@@ -3978,10 +3984,6 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 					require.NoError(t, sst.ClearRawRange(keys.RaftHardStateKey(rangeID), s.EndKey, true, false))
 				}
 
-				require.NoError(t, kvstorage.MakeStateLoader(rangeID).SetRangeTombstone(
-					context.Background(), &sst,
-					kvserverpb.RangeTombstone{NextReplicaID: math.MaxInt32},
-				))
 				require.NoError(t, sst.Finish())
 				expectedSSTs = append(expectedSSTs, sstFile.Data())
 			}
@@ -3995,7 +3997,9 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 				EndKey:   roachpb.RKey(keyEnd),
 			}
 			require.NoError(t, storage.ClearRangeWithHeuristic(
-				ctx, receivingEng, &sst, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey(), 64,
+				ctx, receivingEng, &sst,
+				desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey(),
+				kvstorage.ClearRangeThresholdPointKeys(),
 			))
 			require.NoError(t, sst.Finish())
 			expectedSSTs = append(expectedSSTs, sstFile.Data())

pkg/kv/kvserver/kvstorage/destroy.go

@@ -13,31 +13,37 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/rditer"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage"
+	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
 	"github.com/cockroachdb/errors"
 )
 
-const (
-	// ClearRangeThresholdPointKeys is the threshold (as number of point keys)
-	// beyond which we'll clear range data using a Pebble range tombstone rather
-	// than individual Pebble point tombstones.
-	//
-	// It is expensive for there to be many Pebble range tombstones in the same
-	// sstable because all of the tombstones in an sstable are loaded whenever the
-	// sstable is accessed. So we avoid using range deletion unless there is some
-	// minimum number of keys. The value here was pulled out of thin air. It might
-	// be better to make this dependent on the size of the data being deleted. Or
-	// perhaps we should fix Pebble to handle large numbers of range tombstones in
-	// an sstable better.
-	ClearRangeThresholdPointKeys = 64
-
-	// MergedTombstoneReplicaID is the replica ID written into the RangeTombstone
-	// for replicas of a range which is known to have been merged. This value
-	// should prevent any messages from stale replicas of that range from ever
-	// resurrecting merged replicas. Whenever merging or subsuming a replica we
-	// know new replicas can never be created so this value is used even if we
-	// don't know the current replica ID.
-	MergedTombstoneReplicaID roachpb.ReplicaID = math.MaxInt32
-)
+// MergedTombstoneReplicaID is the replica ID written into the RangeTombstone
+// for replicas of a range which is known to have been merged. This value
+// should prevent any messages from stale replicas of that range from ever
+// resurrecting merged replicas. Whenever merging or subsuming a replica we
+// know new replicas can never be created so this value is used even if we
+// don't know the current replica ID.
+const MergedTombstoneReplicaID roachpb.ReplicaID = math.MaxInt32
+
+// clearRangeThresholdPointKeys is the value of ClearRangeThresholdPointKeys().
+// Can be overridden only in tests, in order to deterministically force using
+// ClearRawRange instead of point clears, when destroying replicas.
+var clearRangeThresholdPointKeys = 64
+
+// ClearRangeThresholdPointKeys returns the threshold (as number of point keys)
+// beyond which we'll clear range data using a Pebble range tombstone rather
+// than individual Pebble point tombstones.
+//
+// It is expensive for there to be many Pebble range tombstones in the same
+// sstable because all of the tombstones in an sstable are loaded whenever the
+// sstable is accessed. So we avoid using range deletion unless there is some
+// minimum number of keys. The value here was pulled out of thin air. It might
+// be better to make this dependent on the size of the data being deleted. Or
+// perhaps we should fix Pebble to handle large numbers of range tombstones in
+// an sstable better.
+func ClearRangeThresholdPointKeys() int {
+	return clearRangeThresholdPointKeys
+}
 
 // clearRangeDataOptions specify which parts of a Replica are to be destroyed.
 type clearRangeDataOptions struct {
@@ -84,7 +90,7 @@ func clearRangeData(
 		UnreplicatedByRangeID: opts.clearUnreplicatedByRangeID,
 	})
 
-	pointKeyThreshold := ClearRangeThresholdPointKeys
+	pointKeyThreshold := ClearRangeThresholdPointKeys()
 	if opts.mustUseClearRange {
 		pointKeyThreshold = 1
 	}
@@ -252,3 +258,21 @@ func RemoveStaleRHSFromSplit(
 		clearUnreplicatedByRangeID: true,
 	})
 }
+
+// TestingForceClearRange changes the value of ClearRangeThresholdPointKeys to
+// 1, which effectively forces ClearRawRange in the replica destruction path,
+// instead of point deletions. This can be used for making the storage
+// operations in tests deterministic.
+//
+// The caller must call the returned function at the end of the test, to restore
+// the default value.
+func TestingForceClearRange() func() {
+	if !buildutil.CrdbTestBuild {
+		panic("test-only function")
+	}
+	old := clearRangeThresholdPointKeys
+	clearRangeThresholdPointKeys = 1 // forces ClearRawRange
+	return func() {
+		clearRangeThresholdPointKeys = old
+	}
+}

pkg/kv/kvserver/snapshot_apply_prepare.go

@@ -225,7 +225,7 @@ func (s *snapWriteBuilder) clearResidualDataOnNarrowSnapshot(ctx context.Context
 	}) {
 		if err := s.writeSST(ctx, func(ctx context.Context, w storage.Writer) error {
 			return storage.ClearRangeWithHeuristic(
-				ctx, reader, w, span.Key, span.EndKey, kvstorage.ClearRangeThresholdPointKeys,
+				ctx, reader, w, span.Key, span.EndKey, kvstorage.ClearRangeThresholdPointKeys(),
 			)
 		}); err != nil {
 			return err