storage: constrained span of rangedel in ClearRange to keys in range

[OwenQian]

Constrains the width of the range deletion tombstone to the span of keys
actually present within the range. If the range has no kv-entries, then skip the
rangedel completely.

Before this change, when receiving a snapshot, the original file
would have a range deletion tombstone that spanned the entire range written to
it regardless of the actual keys contained in the range or if the range was
empty. This resulted in the creation of excessively wide tombstones, which has
significant performance implications since the wide tombstones impede
compaction.

Fixes #44048.

Rebased off #45100.

Release note: None.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Nice, thanks for getting this out @OwenQian. We're going to want to add some testing that looks similar to TestStoreRangeMergeRaftSnapshot but tests a) the case where the recipient has an empty keyspace and b) the case where the recipient has a constrained set of keys in its keyspace. Finally, we'll want to add similar logic to clearSubsumedReplicaDiskData, where you left your TODO.

Also, if you add "Fixes #44048." to your commit message and/or PR message, Github will automatically link the PR to the issue and close the issue when we eventually merge the fix.

@sumeerbhola @petermattis @dt this should be far enough along to unblock you on #44048.

Reviewed 1 of 8 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @OwenQian)

---

pkg/storage/replica_sst_snapshot_storage_test.go, line 52 at r1 (raw file):

	}()

	// Check that even though the files aren't created, they are still recorded in SSTs().

We'll need to update this comment. Then we'll want to test that once we start writing to the files, they get added to the slice.

---

pkg/storage/store_snapshot.go, line 110 at r1 (raw file):

	// Only used on the receiver side.
	scratch *SSTSnapshotStorageScratch
	// Used to restrict the span of the rangedel done in ClearRange when receiving a snapshot

Add a period to the end of this.

---

pkg/storage/store_snapshot.go, line 149 at r1 (raw file):

func (msstw *multiSSTWriter) initSST(ctx context.Context) error {
	span := roachpb.Span{Key: msstw.keyRanges[msstw.currRange].Start.Key, EndKey: msstw.keyRanges[msstw.currRange].End.Key}

Add a comment about what we're doing here.

---

pkg/storage/store_snapshot.go, line 154 at r1 (raw file):

		return nil
	}
	// Only create file if the range is non-empty to avoid ingesting an empty SST later on since range dels are skipped for empty ranges.

Wrap this at 80 characters.

---

pkg/storage/store_snapshot.go, line 202 at r1 (raw file):

		}
	}
	if err := msstw.maybeCreateNewFile(ctx); err != nil {

nit: I'd move this below the assertion. Also, wrap the error with errors.Wrap.

---

pkg/storage/rditer/replica_data_iter.go, line 59 at r1 (raw file):

	defer it.Close()
	it.SeekGE(engine.MakeMVCCMetadataKey(span.Key))
	if valid, _ := it.Valid(); !valid {

We should propagate these errors. If we hit the iterator bounds then we'll get false, nil from Valid, but if we hit a real error like RocksDB corruption, we'll want to infomr the caller.

---

pkg/storage/rditer/replica_data_iter_test.go, line 280 at r1 (raw file):

	span := roachpb.Span{
		Key: roachpb.Key("a"),

I think you need to gofmt this file.

---

pkg/storage/rditer/replica_data_iter_test.go, line 283 at r1 (raw file):

		EndKey:   roachpb.Key("z"),
	}
	if _, empty := ConstrainToKeys(eng, span); !empty {

We've been slowly switching tests over to use the require package. We should use that here. Doing so will make it easy to assert that both return values are what we expect.

---

pkg/storage/rditer/replica_data_iter_test.go, line 313 at r1 (raw file):

	key := keys.TransactionKey(roachpb.Key(desc.StartKey), uuid.MakeV4())
	ts := hlc.Timestamp{}
	if err := engine.MVCCPut(context.Background(), eng, nil, key, ts, roachpb.MakeValueFromString("value"), nil); err != nil {

This is still performing the Put here. Did you determine that it was needed?

[nvanbenschoten]

Another point here is that we generally try to write descriptive commit messages that inform readers of the goal of each change and the approach it took to accomplish that goal. Here's a resource that discusses how to write a good git commit message: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages.

I know I mentioned that we should get this up to unblock people, so it's likely that that explains the temporary brevity. This is just something to keep in mind.

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @OwenQian)

---

pkg/storage/rditer/replica_data_iter.go, line 68 at r5 (raw file):

	it.SeekLT(engine.MakeMVCCMetadataKey(span.EndKey))
	if valid, err := it.Valid(); !valid {
		return roachpb.Span{}, true, err

seems like in this case err != nil must be true, since we know there is a key that is in the span (or we have a bug somewhere). If yes, it may be worth confirming that here and explicitly creating an error if it is nil.

[OwenQian]

---

pkg/storage/rditer/replica_data_iter.go, line 68 at r5 (raw file):

Previously, sumeerbhola wrote…

seems like in this case err != nil must be true, since we know there is a key that is in the span (or we have a bug somewhere). If yes, it may be worth confirming that here and explicitly creating an error if it is nil.

Added explicit error check for when the second iterator scan unexpected returns an error once we already know the span is non-empty.

[OwenQian]

---

pkg/storage/rditer/replica_data_iter.go, line 68 at r5 (raw file):

Previously, OwenQian (Owen Qian) wrote…

Added explicit error check for when the second iterator scan unexpected returns an error once we already know the span is non-empty.

Done.

[OwenQian]

---

pkg/storage/rditer/replica_data_iter.go, line 59 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We should propagate these errors. If we hit the iterator bounds then we'll get false, nil from Valid, but if we hit a real error like RocksDB corruption, we'll want to infomr the caller.

Done.

[OwenQian]

---

pkg/storage/replica_sst_snapshot_storage_test.go, line 52 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We'll need to update this comment. Then we'll want to test that once we start writing to the files, they get added to the slice.

Done.

[OwenQian]

---

pkg/storage/store_snapshot.go, line 110 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add a period to the end of this.

Done.

[OwenQian]

---

pkg/storage/store_snapshot.go, line 149 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add a comment about what we're doing here.

Done.

[OwenQian]

---

pkg/storage/rditer/replica_data_iter_test.go, line 280 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think you need to gofmt this file.

Done.

[OwenQian]

---

pkg/storage/rditer/replica_data_iter_test.go, line 313 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This is still performing the Put here. Did you determine that it was needed?

Done.

[OwenQian]

---

pkg/storage/rditer/replica_data_iter_test.go, line 283 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We've been slowly switching tests over to use the require package. We should use that here. Doing so will make it easy to assert that both return values are what we expect.

Done.

[OwenQian]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)

---

pkg/storage/store_snapshot.go, line 154 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Wrap this at 80 characters.

Done.

[OwenQian]

---

pkg/storage/store_snapshot.go, line 202 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: I'd move this below the assertion. Also, wrap the error with errors.Wrap.

Done.

[nvanbenschoten]

This is looking a lot better. We still have some work to do to get this test solid, but we're getting closer.

Reviewed 3 of 9 files at r7, 1 of 1 files at r8, 5 of 5 files at r9.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @OwenQian and @sumeerbhola)

---

pkg/storage/client_merge_test.go, line 2918 at r9 (raw file):

			sstFile := &engine.MemFile{}
			sst := engine.MakeIngestionSSTWriter(sstFile)
			span, empty, err := rditer.ConstrainToKeys(sendingEng, roachpb.Span{Key: r.Start.Key, EndKey: r.End.Key})

This is similar to what we talked about before. We don't want to re-use the logic we're trying to test in the test itself - that's not a test of anything. If ConstrainToKeys was broken then we would never catch it here, right? This is similar to the following code:

func plus(a, b int) int {
    // Wrong!
    return a * b
}

func TestPlus(t *testing.T) {
    const a, b = 10, 20
    exp := plus(a, b)
    // Will pass, even though plus is completely broken.
    require.Equal(t, exp, plus(a, b))
}

Instead of this, the test should know exactly what this clear range should encompass, and it should add that to the sst.

In fact, I'm realizing that we have the same issue with the rest of this SST. Instead of using the sender's engine to create it, we should be able to predict the contents of the SST ourselves - we were the ones who wrote the data it should contain in:

// ...
for _, key := range []roachpb.Key{roachpb.Key("a"), roachpb.Key("b"), roachpb.Key("c")} {
    if _, pErr := client.SendWrapped(ctx, distSender, incrementArgs(key, 1)); pErr != nil {
        t.Fatal(pErr)
    }
    mtc.waitForValues(key, []int64{1, 1, 1})
}

// ...
for i := 0; i < 10; i++ {
    key := roachpb.Key("d" + strconv.Itoa(i))
    if _, pErr := client.SendWrapped(ctx, distSender, incrementArgs(key, 1)); pErr != nil {
        t.Fatal(pErr)
    }
    mtc.waitForValues(key, []int64{1, 1, 1})
}

---

pkg/storage/replica_sst_snapshot_storage_test.go, line 137 at r9 (raw file):

	sstSnapshotStorage := NewSSTSnapshotStorage(eng, testLimiter)
	var scratch *SSTSnapshotStorageScratch
	// Expect a SST file to be created for a range only if that range contains

Let's add a pair of subtests using testutils.RunTrueAndFalse to test with and without these keys. We also want to test that without these keys, the SSTs are not created.

---

pkg/storage/replica_sst_snapshot_storage_test.go, line 141 at r9 (raw file):

	// three types of replicated key ranges and check to see if the number of
	// SST files created matches what we expect.
	insertedKeys := []roachpb.Key{rangeIDLocalKey, rangeLocalKey, userKey}

This all seems a little fragile. We're trying to line up these keys with corresponding key ranges that are returned by rditer.MakeReplicatedKeyRanges. What if the implementation of rditer.MakeReplicatedKeyRanges changes? What if it returns keys in a different order?

I think we can improve this. Instead of creating these keys, can we just use the .Start key of each key range? Something like:

insertedKeys := make([]engine.MVCCKey, len(keyRanges))
for i, kr := range keyRanges {
    insertedKeys[i] = kr.Start
}

---

pkg/storage/replica_sst_snapshot_storage_test.go, line 142 at r9 (raw file):

	// SST files created matches what we expect.
	insertedKeys := []roachpb.Key{rangeIDLocalKey, rangeLocalKey, userKey}
	for numberOfPuts := 0; numberOfPuts < 4; numberOfPuts++ {

Don't hard-code 4 here. It's a function of the length of keyRanges, right? Then let's use that.

---

pkg/storage/store_snapshot.go, line 110 at r1 (raw file):

Previously, OwenQian (Owen Qian) wrote…

Done.

There are multiple range dels performed, so this comment needs to bit of work.

---

pkg/storage/store_snapshot.go, line 126 at r9 (raw file):

	// before flushing to disk.
	sstChunkSize int64
	// Used to scan for the Upper and LowerBound of ranges to restrict

s/Upper and LowerBound of ranges/minimum and maximum key in the range/

---

pkg/storage/store_snapshot.go, line 127 at r9 (raw file):

	sstChunkSize int64
	// Used to scan for the Upper and LowerBound of ranges to restrict
	// tombstone width.

s/tombstone/range deletion tombstone/

---

pkg/storage/store_snapshot.go, line 151 at r9 (raw file):

func (msstw *multiSSTWriter) initSST(ctx context.Context) error {
	span := roachpb.Span{Key: msstw.keyRanges[msstw.currRange].Start.Key, EndKey: msstw.keyRanges[msstw.currRange].End.Key}

Fold this line to look like:

span := roachpb.Span{
    Key: msstw.keyRanges[msstw.currRange].Start.Key, 
    EndKey: msstw.keyRanges[msstw.currRange].End.Key,
}

---

pkg/storage/store_snapshot.go, line 184 at r9 (raw file):

}

// maybeCreateNewFile creates a new file if currSST is closed. Otherwise it is

nit: move this above finalizeSST.

---

pkg/storage/store_snapshot.go, line 187 at r9 (raw file):

// a no-op. maybeCreateNewFile is idempotent.
func (msstw *multiSSTWriter) maybeCreateNewFile(ctx context.Context) error {
	if !msstw.currSST.Closed() {

Add a small comment like:

// The file was already created.

---

pkg/storage/rditer/replica_data_iter.go, line 62 at r9 (raw file):

	defer it.Close()
	it.SeekGE(engine.MakeMVCCMetadataKey(span.Key))
	if valid, err := it.Valid(); !valid {

Split this into two cases like you have below.

---

pkg/storage/rditer/replica_data_iter.go, line 69 at r9 (raw file):

	it.SeekLT(engine.MakeMVCCMetadataKey(span.EndKey))
	if valid, err := it.Valid(); err != nil {
		return roachpb.Span{}, true, errors.Wrapf(err, "Unexpected error when constraining non-empty span")

s/Unexpected/unexpected/

---

pkg/storage/rditer/replica_data_iter.go, line 71 at r9 (raw file):

		return roachpb.Span{}, true, errors.Wrapf(err, "Unexpected error when constraining non-empty span")
	} else if !valid {
		return roachpb.Span{}, true, err

return roachpb.Span{}, true, nil

---

pkg/storage/rditer/replica_data_iter_test.go, line 303 at r9 (raw file):

	require.False(t, empty)
	require.Equal(t, span.Key, insertedKey)
	require.Equal(t, span.EndKey, insertedKey.Next())

There are a few more cases to add here:

1. after adding a second key
2. after adding a key past the end of the search span
3. after adding a key before the start of the search span

[OwenQian]

---

pkg/storage/client_merge_test.go, line 2918 at r9 (raw file):

This is similar to what we talked about before. We don't want to re-use the logic we're trying to test in the test itself - that's not a test of anything.

I might be confusing concepts here, but I'm curious about your thoughts on testing approach here:

Wouldn't a malfunction in ConstrainForKeys be caught in one of the unit tests for ConstrainForKeys e.g. TestConstrainToKeysNonEmptyRange, and so for the purpose of this integration test, we can assume that if those tests are passing, then ConstrainForKeys has the behavior we expect it to? Or is the preferred approach that we still want to minimize the number of assumptions that we make about underlying implementation even given the unit test?

I'm thinking if we used a separate code path to generate the expected SSTs that would couple the test tightly with the underlying implementation. Then if the behavior of ConstrainToKeys changed but we semantically still wanted to "constrain to keys", this test would break (along with the ConstrainToKeys unit tests).

I know we talked offline about the three different levels of testing and how CRDB wasn't big on mocking/unit tests. Sorry if I'm mixing up concepts here, I definitely do see how the current set-up is a bit tautological, so some extra clarity here would be much appreciated.

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @OwenQian and @sumeerbhola)

---

pkg/storage/client_merge_test.go, line 2918 at r9 (raw file):

Wouldn't a malfunction in ConstrainForKeys be caught in one of the unit tests for ConstrainForKeys

Ideally, it would, yes. We use unit tests to ensure that a given piece of logic produces expected results over its input domain. But in addition to unit tests, we also want to make assertions across multiple layers to strengthen our confidence that each component is interacting correctly and that we weren't missing any unit tests. Testing at only a single layer leads to blind spots.

Or is the preferred approach that we still want to minimize the number of assumptions that we make about underlying implementation even given the unit test?

Yes, to some degree. We certainly need to make assumptions when writing tests, but we also want to make sure we're making strong assertions which is only possible by avoiding assuming too much.

I'm thinking if we used a separate code path to generate the expected SSTs that would couple the test tightly with the underlying implementation.

We seem to be thinking about what constitutes "implementation details" differently. This test is verifying that the data ingested during a Range snapshot is what we expect it to be. So from the perspective of this test, the entries that are ingested are the entire point - not an implementation detail of the range snapshot mechanism. The same applies to this range deletion tombstone. From the perspective of the test, the bounds of the range deletion tombstone that is ingested is the important part, not that there is a range deletion tombstone that meets some abstract "constrained to keys" notion. Put another way, this test should fail if we changed the semantics of ConstrainToKeys.

Arguments could be made either way here though and there's not a hard rule on what should and shouldn't be assumed. I think the best advice I can give is that tests should convince their reader that the functionality they are testing is correct. Each assumption we make in tests works against this goal.

[OwenQian]

---

pkg/storage/client_merge_test.go, line 2918 at r9 (raw file):

In fact, I'm realizing that we have the same issue with the rest of this SST. Instead of using the sender's engine to create it, we should be able to predict the contents of the SST ourselves - we were the ones who wrote the data it should contain in:

Updated test to generate expected SST from scratch rather than relying on the sender's engine and removed dependency on ConstrainToKeys.

[OwenQian]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)

---

pkg/storage/replica_sst_snapshot_storage_test.go, line 137 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's add a pair of subtests using testutils.RunTrueAndFalse to test with and without these keys. We also want to test that without these keys, the SSTs are not created.

Done.

---

pkg/storage/replica_sst_snapshot_storage_test.go, line 141 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This all seems a little fragile. We're trying to line up these keys with corresponding key ranges that are returned by rditer.MakeReplicatedKeyRanges. What if the implementation of rditer.MakeReplicatedKeyRanges changes? What if it returns keys in a different order?

I think we can improve this. Instead of creating these keys, can we just use the .Start key of each key range? Something like:

insertedKeys := make([]engine.MVCCKey, len(keyRanges))
for i, kr := range keyRanges {
    insertedKeys[i] = kr.Start
}

Ooh, yeah I that a lot better, done.

---

pkg/storage/replica_sst_snapshot_storage_test.go, line 142 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Don't hard-code 4 here. It's a function of the length of keyRanges, right? Then let's use that.

Done.

---

pkg/storage/store_snapshot.go, line 110 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

There are multiple range dels performed, so this comment needs to bit of work.

Updated comment to add more details.

---

pkg/storage/store_snapshot.go, line 126 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

s/Upper and LowerBound of ranges/minimum and maximum key in the range/

Done.

---

pkg/storage/store_snapshot.go, line 127 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

s/tombstone/range deletion tombstone/

Done.

---

pkg/storage/store_snapshot.go, line 151 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Fold this line to look like:

span := roachpb.Span{
    Key: msstw.keyRanges[msstw.currRange].Start.Key, 
    EndKey: msstw.keyRanges[msstw.currRange].End.Key,
}

Done.

---

pkg/storage/store_snapshot.go, line 184 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: move this above finalizeSST.

Done.

---

pkg/storage/store_snapshot.go, line 187 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add a small comment like:

// The file was already created.

Done.

---

pkg/storage/rditer/replica_data_iter.go, line 62 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Split this into two cases like you have below.

Done.

---

pkg/storage/rditer/replica_data_iter.go, line 69 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

s/Unexpected/unexpected/

Done.

---

pkg/storage/rditer/replica_data_iter.go, line 71 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

return roachpb.Span{}, true, nil

Done.

---

pkg/storage/rditer/replica_data_iter_test.go, line 303 at r9 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

There are a few more cases to add here:

1. after adding a second key
2. after adding a key past the end of the search span
3. after adding a key before the start of the search span

Done.

[nvanbenschoten]

This is looking significantly better. I think we'll be good to merge after addressing a few more small nits and stabilizing CI. Nice job!

Reviewed 17 of 18 files at r10.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @OwenQian and @sumeerbhola)

---

pkg/kv/kvserver/client_merge_test.go, line 2854 at r10 (raw file):

}

// TestStoreRangeMergeRaftSnapshot sets up a situation which triggeres a

triggers

---

pkg/kv/kvserver/client_merge_test.go, line 2854 at r10 (raw file):

}

// TestStoreRangeMergeRaftSnapshot sets up a situation which triggeres a

Nice comment!

---

pkg/kv/kvserver/client_merge_test.go, line 2880 at r10 (raw file):

	// Used to set the MVCCKey.Timestamp of the manually inserted keys so we
	// can generate a byte-by-byte equal expected SST without relying on the
	// sender for anything other than the timestamps.

We're not really relying on the sender for this, are we? We generated these keys.

---

pkg/kv/kvserver/client_merge_test.go, line 2881 at r10 (raw file):

	// can generate a byte-by-byte equal expected SST without relying on the
	// sender for anything other than the timestamps.
	tsMap := make(map[string]hlc.Timestamp)

nit: rename this to something like keyTimestamps.

---

pkg/kv/kvserver/client_merge_test.go, line 2938 at r10 (raw file):

			sst := engine.MakeIngestionSSTWriter(sstFile)
			// Expect the user KeyRange to be [a, d) but we only have keys at
			// a, b and c so expect the range del tombstone to span [a, c).

Reference ConstrainToKeys here.

---

pkg/kv/kvserver/client_merge_test.go, line 3048 at r10 (raw file):

			t.Fatal(pErr)
		}
		tsMap[string(key)] = br.BatchResponse_Header.Timestamp

nit: BatchResponse_Header is embedded, so you can leave it out here.

---

pkg/kv/kvserver/rditer/replica_data_iter_test.go, line 288 at r10 (raw file):

	defer eng.Close()

	startKey := roachpb.Key("b")

nit: I think the test would actually be a lot easier to read if you inlined all of these variables. Want to try that out and see how it looks? I'll defer to your judgment.

[OwenQian]

---

pkg/kv/kvserver/client_merge_test.go, line 2881 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: rename this to something like keyTimestamps.

Done.

[OwenQian]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)

---

pkg/kv/kvserver/client_merge_test.go, line 2854 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

triggers

Done.

---

pkg/kv/kvserver/client_merge_test.go, line 2854 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Nice comment!

Thanks!

---

pkg/kv/kvserver/client_merge_test.go, line 2880 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We're not really relying on the sender for this, are we? We generated these keys.

Updated comment.

---

pkg/kv/kvserver/client_merge_test.go, line 2938 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Reference ConstrainToKeys here.

Done.

---

pkg/kv/kvserver/client_merge_test.go, line 3048 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: BatchResponse_Header is embedded, so you can leave it out here.

Done.

---

pkg/kv/kvserver/rditer/replica_data_iter_test.go, line 288 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: I think the test would actually be a lot easier to read if you inlined all of these variables. Want to try that out and see how it looks? I'll defer to your judgment.

I inlined the keys and added brief comments to explain why those keys were chosen.

[nvanbenschoten]

let's get this merged!

Reviewed 6 of 6 files at r11.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @OwenQian and @sumeerbhola)

---

pkg/kv/kvserver/client_merge_test.go, line 2939 at r11 (raw file):

			// a, b and c so we expect rditer.ConstrainToKeys to restrict the
			// width of the range del tombstone to [a, c).
			if err := sst.ClearRange(storage.MakeMVCCMetadataKey(roachpb.Key("a")), storage.MakeMVCCMetadataKey(roachpb.Key("c").Next())); err != nil {

nit: wrap this line like:

if err := sst.ClearRange(
    storage.MakeMVCCMetadataKey(roachpb.Key("a")), 
    storage.MakeMVCCMetadataKey(roachpb.Key("c").Next()),
); err != nil {

[OwenQian]

---

pkg/kv/kvserver/client_merge_test.go, line 2939 at r11 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: wrap this line like:

if err := sst.ClearRange(
    storage.MakeMVCCMetadataKey(roachpb.Key("a")), 
    storage.MakeMVCCMetadataKey(roachpb.Key("c").Next()),
); err != nil {

Done.

[OwenQian]

bors r=nvanbenschoten

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)