kv: complete client range info migrations

This commit completes a series of migrations relating to RangeInfo state
and the client/server protocol for keeping the range descriptor cache up
to date. The migrations were started in cockroachdb#51168, cockroachdb#51378, cockroachdb#51437.

Concretely, this commit removes the following cluster versions:
- `ClientRangeInfosOnBatchResponse`
- `RangeStatsRespHasDesc`

It addresses 6 TODOs.

And it makes the following changes to the KV APi:
- makes `Header.ClientRangeInfo` non-nullable.
- deletes `Header.ReturnRangeInfo`.
- deletes `ResponseHeader.DeprecatedRangeInfos`.
- stops propagating `BatchResponse.RangeInfos` past `DistSender`.
- makes RangeStatsResponse.RangeInfo non-nullable.

pkg/clusterversion/cockroach_versions.go

@@ -158,14 +158,9 @@ const (
 	// NoOriginFKIndexes allows for foreign keys to no longer need indexes on
 	// the origin side of the relationship.
 	NoOriginFKIndexes
-	// ClientRangeInfosOnBatchResponse moves the response RangeInfos from
-	// individual response headers to the batch header.
-	ClientRangeInfosOnBatchResponse
 	// NodeMembershipStatus gates the usage of the MembershipStatus enum in the
 	// Liveness proto. See comment on proto definition for more details.
 	NodeMembershipStatus
-	// RangeStatsRespHasDesc adds the RangeStatsResponse.RangeInfo field.
-	RangeStatsRespHasDesc
 	// MinPasswordLength adds the server.user_login.min_password_length setting.
 	MinPasswordLength
 	// AbortSpanBytes adds a field to MVCCStats
@@ -286,18 +281,10 @@ var versionsSingleton = keyedVersions([]keyedVersion{
 		Key:     NoOriginFKIndexes,
 		Version: roachpb.Version{Major: 20, Minor: 1, Internal: 9},
 	},
-	{
-		Key:     ClientRangeInfosOnBatchResponse,
-		Version: roachpb.Version{Major: 20, Minor: 1, Internal: 10},
-	},
 	{
 		Key:     NodeMembershipStatus,
 		Version: roachpb.Version{Major: 20, Minor: 1, Internal: 11},
 	},
-	{
-		Key:     RangeStatsRespHasDesc,
-		Version: roachpb.Version{Major: 20, Minor: 1, Internal: 12},
-	},
 	{
 		Key:     MinPasswordLength,
 		Version: roachpb.Version{Major: 20, Minor: 1, Internal: 13},

pkg/kv/kvclient/kvcoord/BUILD.bazel

@@ -34,7 +34,6 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/base",
-        "//pkg/clusterversion",
         "//pkg/gossip",
         "//pkg/keys",
         "//pkg/kv",

pkg/kv/kvclient/kvcoord/dist_sender.go

@@ -20,7 +20,6 @@ import (
 	"unsafe"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
-	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/gossip"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
@@ -1554,17 +1553,6 @@ func (ds *DistSender) sendPartialBatch(
 
 		// If sending succeeded, return immediately.
 		if reply.Error == nil {
-			// 20.1 nodes return RangeInfos in individual responses. Let's move it to
-			// the br.
-			if ba.ReturnRangeInfo &&
-				len(reply.RangeInfos) == 0 &&
-				!ds.st.Version.IsActive(ctx, clusterversion.ClientRangeInfosOnBatchResponse) {
-				// All the responses have the same RangeInfos in them, so just look at the
-				// first one.
-				firstRes := reply.Responses[0].GetInner()
-				reply.RangeInfos = append(reply.RangeInfos, firstRes.Header().DeprecatedRangeInfos...)
-			}
-
 			return response{reply: reply, positions: positions}
 		}
 
@@ -1870,7 +1858,7 @@ func (ds *DistSender) sendToReplicas(
 		prevReplica = curReplica
 		// Communicate to the server the information our cache has about the range.
 		// If it's stale, the serve will return an update.
-		ba.ClientRangeInfo = &roachpb.ClientRangeInfo{
+		ba.ClientRangeInfo = roachpb.ClientRangeInfo{
 			// Note that DescriptorGeneration will be 0 if the cached descriptor is
 			// "speculative" (see DescSpeculative()). Even if the speculation is
 			// correct, we want the serve to return an update, at which point the
@@ -1973,12 +1961,12 @@ func (ds *DistSender) sendToReplicas(
 
 			if br.Error == nil {
 				// If the server gave us updated range info, lets update our cache with it.
-				//
-				// TODO(andreimatei): shouldn't we do this unconditionally? Our cache knows how
-				// to disregard stale information.
 				if len(br.RangeInfos) > 0 {
 					log.VEventf(ctx, 2, "received updated range info: %s", br.RangeInfos)
 					routing.EvictAndReplace(ctx, br.RangeInfos...)
+					// The field is cleared by the DistSender because it refers
+					// routing information not exposed by the KV API.
+					br.RangeInfos = nil
 				}
 				return br, nil
 			}

pkg/kv/kvserver/batcheval/cmd_range_stats.go

@@ -44,6 +44,6 @@ func RangeStats(
 	reply.MVCCStats = cArgs.EvalCtx.GetMVCCStats()
 	reply.QueriesPerSecond = cArgs.EvalCtx.GetSplitQPS()
 	desc, lease := cArgs.EvalCtx.GetDescAndLease(ctx)
-	reply.RangeInfo = &roachpb.RangeInfo{Desc: desc, Lease: lease}
+	reply.RangeInfo = roachpb.RangeInfo{Desc: desc, Lease: lease}
 	return result.Result{}, nil
 }

pkg/kv/kvserver/client_replica_test.go

@@ -1633,176 +1633,6 @@ func TestErrorHandlingForNonKVCommand(t *testing.T) {
 	}
 }
 
-func TestRangeInfo(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-
-	ctx := context.Background()
-	tc := testcluster.StartTestCluster(t, 2, base.TestClusterArgs{
-		ReplicationMode: base.ReplicationManual,
-	})
-	defer tc.Stopper().Stop(ctx)
-
-	// Split the key space at key "a".
-	minKey := tc.ScratchRange(t)
-	splitKey := minKey.Next().Next()
-	tc.SplitRangeOrFatal(t, splitKey)
-	tc.AddVotersOrFatal(t, minKey, tc.Target(1))
-	tc.AddVotersOrFatal(t, splitKey, tc.Target(1))
-
-	// Get the replicas for each side of the split. This is done within
-	// a SucceedsSoon loop to ensure the split completes.
-	var lhsReplica0, lhsReplica1, rhsReplica0, rhsReplica1 *kvserver.Replica
-	testutils.SucceedsSoon(t, func() error {
-		lhsReplica0 = tc.GetFirstStoreFromServer(t, 0).LookupReplica(roachpb.RKey(minKey))
-		lhsReplica1 = tc.GetFirstStoreFromServer(t, 1).LookupReplica(roachpb.RKey(minKey))
-		rhsReplica0 = tc.GetFirstStoreFromServer(t, 0).LookupReplica(roachpb.RKey(splitKey))
-		rhsReplica1 = tc.GetFirstStoreFromServer(t, 1).LookupReplica(roachpb.RKey(splitKey))
-		if lhsReplica0 == rhsReplica0 || lhsReplica1 == rhsReplica1 {
-			return errors.Errorf("replicas not post-split %v, %v, %v, %v",
-				lhsReplica0, rhsReplica0, rhsReplica0, rhsReplica1)
-		}
-		return nil
-	})
-	lhsLease, _ := lhsReplica0.GetLease()
-	rhsDesc, rhsLease := rhsReplica0.GetDescAndLease(ctx)
-
-	send := func(args roachpb.Request, returnRangeInfo bool, txn *roachpb.Transaction) *roachpb.BatchResponse {
-		ba := roachpb.BatchRequest{
-			Header: roachpb.Header{
-				ReturnRangeInfo: returnRangeInfo,
-				Txn:             txn,
-			},
-		}
-		ba.Add(args)
-
-		br, pErr := tc.Servers[0].DistSender().Send(ctx, ba)
-		if pErr != nil {
-			t.Fatal(pErr)
-		}
-		return br
-	}
-
-	// Populate the range cache so that the request will be sent to the right
-	// leaseholder, and it will have the up-to-date ClientRangeInfo populated.
-	tc.Servers[0].DistSender().RangeDescriptorCache().Insert(ctx,
-		roachpb.RangeInfo{Desc: rhsDesc, Lease: rhsLease})
-
-	// Verify range info is not set if the request is sent with up-to-date
-	// ClientRangeInfo.
-	getArgs := getArgs(splitKey)
-	br := send(getArgs, false /* returnRangeInfo */, nil /* txn */)
-	if len(br.RangeInfos) > 0 {
-		t.Fatalf("expected empty range infos if unrequested; got %v", br.RangeInfos)
-	}
-
-	// Verify range info on a get request.
-	br = send(getArgs, true /* returnRangeInfo */, nil /* txn */)
-	expRangeInfos := []roachpb.RangeInfo{
-		{
-			Desc:  *rhsReplica0.Desc(),
-			Lease: rhsLease,
-		},
-	}
-	if !reflect.DeepEqual(br.RangeInfos, expRangeInfos) {
-		t.Errorf("on get reply, expected %+v; got %+v", expRangeInfos, br.RangeInfos)
-	}
-
-	// Verify range info on a put request.
-	br = send(putArgs(splitKey, []byte("foo")), true /* returnRangeInfo */, nil /* txn */)
-	if !reflect.DeepEqual(br.RangeInfos, expRangeInfos) {
-		t.Errorf("on put reply, expected %+v; got %+v", expRangeInfos, br.RangeInfos)
-	}
-
-	// Verify range info on an admin request.
-	adminArgs := &roachpb.AdminTransferLeaseRequest{
-		RequestHeader: roachpb.RequestHeader{
-			Key: splitKey,
-		},
-		Target: rhsLease.Replica.StoreID,
-	}
-	br = send(adminArgs, true /* returnRangeInfo */, nil /* txn */)
-	if !reflect.DeepEqual(br.RangeInfos, expRangeInfos) {
-		t.Errorf("on admin reply, expected %+v; got %+v", expRangeInfos, br.RangeInfos)
-	}
-
-	// Verify multiple range infos on a scan request.
-	scanArgs := &roachpb.ScanRequest{
-		RequestHeader: roachpb.RequestHeader{
-			Key:    minKey,
-			EndKey: roachpb.KeyMax,
-		},
-	}
-	txn := roachpb.MakeTransaction("test", minKey, 1, tc.Servers[0].Clock().Now(), 0)
-	br = send(scanArgs, true /* returnRangeInfo */, &txn)
-	expRangeInfos = []roachpb.RangeInfo{
-		{
-			Desc:  *lhsReplica0.Desc(),
-			Lease: lhsLease,
-		},
-		{
-			Desc:  *rhsReplica0.Desc(),
-			Lease: rhsLease,
-		},
-	}
-	if !reflect.DeepEqual(br.RangeInfos, expRangeInfos) {
-		t.Errorf("on scan reply, expected %+v; got %+v", expRangeInfos, br.RangeInfos)
-	}
-
-	// Verify multiple range infos and order on a reverse scan request.
-	revScanArgs := &roachpb.ReverseScanRequest{
-		RequestHeader: roachpb.RequestHeader{
-			Key:    minKey,
-			EndKey: roachpb.KeyMax,
-		},
-	}
-	br = send(revScanArgs, true /* returnRangeInfo */, &txn)
-	expRangeInfos = []roachpb.RangeInfo{
-		{
-			Desc:  *lhsReplica0.Desc(),
-			Lease: lhsLease,
-		},
-		{
-			Desc:  *rhsReplica0.Desc(),
-			Lease: rhsLease,
-		},
-	}
-	if !reflect.DeepEqual(br.RangeInfos, expRangeInfos) {
-		t.Errorf("on reverse scan reply, expected %+v; got %+v", expRangeInfos, br.RangeInfos)
-	}
-
-	// Change lease holders for both ranges and re-scan.
-	for _, r := range []*kvserver.Replica{lhsReplica1, rhsReplica1} {
-		replDesc, err := r.GetReplicaDescriptor()
-		if err != nil {
-			t.Fatal(err)
-		}
-		if err = tc.GetFirstStoreFromServer(t, 0).DB().AdminTransferLease(context.Background(),
-			r.Desc().StartKey.AsRawKey(), replDesc.StoreID); err != nil {
-			t.Fatalf("unable to transfer lease to replica %s: %+v", r, err)
-		}
-	}
-	br = send(scanArgs, true /* returnRangeInfo */, &txn)
-	// Read the expected lease from replica0 rather than replica1 as it may serve
-	// a follower read which will contain the new lease information before
-	// replica1 has applied the lease transfer.
-	lhsLease, _ = lhsReplica0.GetLease()
-	rhsLease, _ = rhsReplica0.GetLease()
-	expRangeInfos = []roachpb.RangeInfo{
-		{
-			Desc:  *lhsReplica1.Desc(),
-			Lease: lhsLease,
-		},
-		{
-			Desc:  *rhsReplica1.Desc(),
-			Lease: rhsLease,
-		},
-	}
-	if !reflect.DeepEqual(br.RangeInfos, expRangeInfos) {
-		t.Errorf("on scan reply, expected %+v; got %+v", expRangeInfos, br.RangeInfos)
-	}
-}
-
 // Test that, if a client makes a request to a range that has recently split and
 // the client indicates that it has pre-split info, the serve replies with
 // updated info on both sides of the split. The server has a heuristic for
@@ -1854,7 +1684,7 @@ func TestRangeInfoAfterSplit(t *testing.T) {
 			ba := roachpb.BatchRequest{
 				Header: roachpb.Header{
 					RangeID: tc.rangeID,
-					ClientRangeInfo: &roachpb.ClientRangeInfo{
+					ClientRangeInfo: roachpb.ClientRangeInfo{
 						DescriptorGeneration: preSplitDesc.Generation,
 					},
 				},
@@ -3560,20 +3390,20 @@ func TestDiscoverIntentAcrossLeaseTransferAwayAndBack(t *testing.T) {
 	require.NoError(t, <-err4C)
 }
 
-// getRangeInfo retreives range info by performing a get against the provided
-// key and setting the ReturnRangeInfo flag to true.
+// getRangeInfo retrieves range info by performing a RangeStatsRequest against
+// the provided key.
 func getRangeInfo(
 	ctx context.Context, db *kv.DB, key roachpb.Key,
 ) (ri *roachpb.RangeInfo, err error) {
 	err = db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
 		b := txn.NewBatch()
-		b.Header.ReturnRangeInfo = true
-		b.AddRawRequest(roachpb.NewGet(key))
+		b.AddRawRequest(&roachpb.RangeStatsRequest{
+			RequestHeader: roachpb.RequestHeader{Key: key},
+		})
 		if err = db.Run(ctx, b); err != nil {
 			return err
 		}
-		resp := b.RawResponse()
-		ri = &resp.RangeInfos[0]
+		ri = &b.RawResponse().Responses[0].GetRangeStats().RangeInfo
 		return nil
 	})
 	return ri, err