Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kv: complete client range info migrations #59395

Merged
merged 1 commit into from
Jan 26, 2021
Merged

kv: complete client range info migrations #59395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions pkg/clusterversion/cockroach_versions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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},
Expand Down
42 changes: 20 additions & 22 deletions pkg/clusterversion/key_string.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion pkg/kv/kvclient/kvcoord/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//pkg/base",
"//pkg/clusterversion",
"//pkg/gossip",
"//pkg/keys",
"//pkg/kv",
Expand Down
20 changes: 4 additions & 16 deletions pkg/kv/kvclient/kvcoord/dist_sender.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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}
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/batcheval/cmd_range_stats.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
184 changes: 7 additions & 177 deletions pkg/kv/kvserver/client_replica_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
},
},
Expand Down Expand Up @@ -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
Expand Down
Loading