storage: fix TestStoreRangeSplitAtRangeBounds

`TestStoreRangeSplitAtRangeBounds` should have been in conflict with cockroachdb#14273,
but the test had been broken for a while because we were sending
`AdminSplit` requests to the wrong range. This change fixes the test by
asserting that attempting to split a range at its start key is a no-op
which does not actually perform a split.

Additionally, we had places in our code that claimed an `AdminSplit`
request to the end of a range was a no-op and other places that claimed
it was an error. In reality, it should have been a `RangeKeyMismatchError`,
so this change removes the incorrect code/comments and asserts this behavior
more directly in the test.

Note that the change in `replica_command.go` does not alter current behavior.
The change simply removes an impossible condition.

pkg/storage/client_split_test.go

@@ -271,11 +271,12 @@ func TestStoreRangeSplitIntents(t *testing.T) {
 	}
 }
 
-// TestStoreRangeSplitAtRangeBounds verifies a range cannot be split
-// at its start or end keys (would create zero-length range!). This
-// sort of thing might happen in the wild if two split requests
-// arrived for same key. The first one succeeds and second would try
-// to split at the start of the newly split range.
+// TestStoreRangeSplitAtRangeBounds verifies that attempting to
+// split a range at its start key is a no-op and does not actually
+// perform a split (would create zero-length range!). This sort
+// of thing might happen in the wild if two split requests arrived for
+// same key. The first one succeeds and second would try to split
+// at the start of the newly split range.
 func TestStoreRangeSplitAtRangeBounds(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	storeCfg := storage.TestStoreConfig(nil)
@@ -284,18 +285,34 @@ func TestStoreRangeSplitAtRangeBounds(t *testing.T) {
 	defer stopper.Stop(context.TODO())
 	store := createTestStoreWithConfig(t, stopper, storeCfg)
 
-	args := adminSplitArgs(roachpb.Key("a"))
-	if _, err := client.SendWrapped(context.Background(), rg1(store), args); err != nil {
-		t.Fatal(err)
+	// Split range 1 at an arbitrary key.
+	key := roachpb.Key("a")
+	h := roachpb.Header{RangeID: 1}
+	args := adminSplitArgs(key)
+	if _, pErr := client.SendWrappedWith(context.Background(), store, h, args); pErr != nil {
+		t.Fatal(pErr)
 	}
-	// This second split will try to split at end of first split range.
-	if _, err := client.SendWrapped(context.Background(), rg1(store), args); err == nil {
-		t.Fatalf("split succeeded unexpectedly")
+	replCount := store.ReplicaCount()
+
+	// An AdminSplit request sent to the end of the old range
+	// should fail with a RangeKeyMismatchError.
+	_, pErr := client.SendWrappedWith(context.Background(), store, h, args)
+	if _, ok := pErr.GetDetail().(*roachpb.RangeKeyMismatchError); !ok {
+		t.Fatalf("expected RangeKeyMismatchError, found: %v", pErr)
 	}
-	// Now try to split at start of new range.
-	args = adminSplitArgs(roachpb.Key("a"))
-	if _, err := client.SendWrapped(context.Background(), rg1(store), args); err == nil {
-		t.Fatalf("split succeeded unexpectedly")
+
+	// An AdminSplit request sent to the start of the new range
+	// should succeed but no new ranges should be created.
+	newRng := store.LookupReplica(roachpb.RKey(key), nil)
+	h.RangeID = newRng.RangeID
+	if _, pErr := client.SendWrappedWith(context.Background(), store, h, args); pErr != nil {
+		t.Fatal(pErr)
+	}
+
+	newReplCount := store.ReplicaCount()
+	if replCount != newReplCount {
+		t.Fatalf("splitting at a range boundary should not create a new range; before second split "+
+			"found %d ranges, after second split found %d ranges", replCount, newReplCount)
 	}
 }
 

pkg/storage/replica_command.go

@@ -2634,10 +2634,9 @@ func (r *Replica) adminSplitWithDescriptor(
 		}
 	}
 
-	// First verify this condition so that it will not return
-	// roachpb.NewRangeKeyMismatchError if splitKey equals to desc.EndKey,
-	// otherwise it will cause infinite retry loop.
-	if desc.StartKey.Equal(splitKey) || desc.EndKey.Equal(splitKey) {
+	// If the range starts at the splitKey, we treat the AdminSplit
+	// as a no-op and return success instead of throwing an error.
+	if desc.StartKey.Equal(splitKey) {
 		log.Event(ctx, "range already split")
 		return reply, false, nil
 	}