diff --git a/pkg/kv/kvclient/kvcoord/dist_sender.go b/pkg/kv/kvclient/kvcoord/dist_sender.go index 9bead2e18321..fbd80fc812a8 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender.go @@ -2014,12 +2014,6 @@ func (ds *DistSender) sendToReplicas( // part of routing.entry.Desc. The transport starts up initialized with // routing's replica info, but routing can be updated as we go through the // replicas, whereas transport isn't. - // - // TODO(andrei): The structure around here is no good; we're potentially - // updating routing with replicas that are not part of transport, and so - // those replicas will never be tried. Instead, we'll exhaust the transport - // and bubble up a SendError, which will cause a cache eviction and a new - // descriptor lookup potentially unnecessarily. lastErr := err if lastErr == nil && br != nil { lastErr = br.Error.GoError() @@ -2201,11 +2195,9 @@ func (ds *DistSender) sendToReplicas( var updatedLeaseholder bool if tErr.Lease != nil { - updatedLeaseholder = routing.UpdateLease(ctx, tErr.Lease, tErr.RangeDesc.Generation) + updatedLeaseholder = routing.SyncTokenAndMaybeUpdateCache(ctx, tErr.Lease, &tErr.RangeDesc) } else if tErr.LeaseHolder != nil { - // tErr.LeaseHolder might be set when tErr.Lease isn't. - routing.UpdateLeaseholder(ctx, *tErr.LeaseHolder, tErr.RangeDesc.Generation) - updatedLeaseholder = true + updatedLeaseholder = routing.SyncTokenAndMaybeUpdateCacheWithSpeculativeLease(ctx, *tErr.LeaseHolder, &tErr.RangeDesc) } // Move the new leaseholder to the head of the queue for the next // retry. Note that the leaseholder might not be the one indicated by @@ -2219,7 +2211,25 @@ func (ds *DistSender) sendToReplicas( // lease expires and someone else gets a new one, so by moving on we // get out of possibly infinite loops. if !lh.IsSame(curReplica) || sameReplicaRetries < sameReplicaRetryLimit { - transport.MoveToFront(*lh) + moved := transport.MoveToFront(*lh) + if !moved { + // The transport always includes the client's view of the + // leaseholder when it's constructed. If the leaseholder can't + // be found on the transport then it must be the case that the + // routing was updated with lease information that is not + // compatible with the range descriptor that was used to + // construct the transport. A client may have an arbitrarily + // stale view of the leaseholder, but it is never expected to + // regress. As such, advancing through each replica on the + // transport until it's exhausted is unlikely to achieve much. + // + // We bail early by returning a SendError. The expectation is + // for the client to retry with a fresher eviction token. + log.VEventf( + ctx, 2, "transport incompatible with updated routing; bailing early", + ) + return nil, newSendError(fmt.Sprintf("leaseholder not found in transport; last error: %s", tErr.Error())) + } } } // Check whether the request was intentionally sent to a follower @@ -2234,6 +2244,15 @@ func (ds *DistSender) sendToReplicas( // the leaseholder, we backoff because it might be the case that // there's a lease transfer in progress and the would-be leaseholder // has not yet applied the new lease. + // + // TODO(arul): The idea here is for the client to not keep sending + // the would-be leaseholder multiple requests and backoff a bit to let + // it apply the its lease. Instead of deriving this information like + // we do above, we could instead check if we're retrying the same + // leaseholder (i.e, if the leaseholder on the routing is the same as + // the replica we just tried), in which case we should backoff. With + // this scheme we'd no longer have to track "updatedLeaseholder" state + // when syncing the NLHE with the range cache. shouldBackoff := !updatedLeaseholder && !intentionallySentToFollower if shouldBackoff { ds.metrics.InLeaseTransferBackoffs.Inc(1) diff --git a/pkg/kv/kvclient/kvcoord/dist_sender_test.go b/pkg/kv/kvclient/kvcoord/dist_sender_test.go index c2c1eff5b3a0..876efd74f21c 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender_test.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender_test.go @@ -62,9 +62,10 @@ var ( testMetaEndKey = roachpb.RKey(keys.SystemPrefix) // single meta1 and meta2 range with one replica. TestMetaRangeDescriptor = roachpb.RangeDescriptor{ - RangeID: 1, - StartKey: roachpb.RKeyMin, - EndKey: testMetaEndKey, + RangeID: 1, + Generation: 1, + StartKey: roachpb.RKeyMin, + EndKey: testMetaEndKey, InternalReplicas: []roachpb.ReplicaDescriptor{ { NodeID: 1, @@ -78,9 +79,10 @@ var ( // // single user-space descriptor with one replica. testUserRangeDescriptor = roachpb.RangeDescriptor{ - RangeID: 2, - StartKey: testMetaEndKey, - EndKey: roachpb.RKeyMax, + RangeID: 2, + Generation: 1, + StartKey: testMetaEndKey, + EndKey: roachpb.RKeyMax, InternalReplicas: []roachpb.ReplicaDescriptor{ { NodeID: 1, @@ -90,9 +92,10 @@ var ( } // single user-space descriptor with three replicas. testUserRangeDescriptor3Replicas = roachpb.RangeDescriptor{ - RangeID: 2, - StartKey: testMetaEndKey, - EndKey: roachpb.RKeyMax, + RangeID: 2, + Generation: 1, + StartKey: testMetaEndKey, + EndKey: roachpb.RKeyMax, InternalReplicas: []roachpb.ReplicaDescriptor{ { ReplicaID: 1, @@ -187,7 +190,7 @@ func (l *simpleTransportAdapter) SkipReplica() { l.nextReplicaIdx++ } -func (l *simpleTransportAdapter) MoveToFront(replica roachpb.ReplicaDescriptor) { +func (l *simpleTransportAdapter) MoveToFront(replica roachpb.ReplicaDescriptor) bool { for i := range l.replicas { if l.replicas[i].IsSame(replica) { // If we've already processed the replica, decrement the current @@ -197,9 +200,10 @@ func (l *simpleTransportAdapter) MoveToFront(replica roachpb.ReplicaDescriptor) } // Swap the client representing this replica to the front. l.replicas[i], l.replicas[l.nextReplicaIdx] = l.replicas[l.nextReplicaIdx], l.replicas[i] - return + return true } } + return false } func (l *simpleTransportAdapter) Release() {} @@ -589,8 +593,9 @@ func TestRetryOnNotLeaseHolderError(t *testing.T) { { name: "leaseholder in desc", nlhe: roachpb.NotLeaseHolderError{ - RangeID: testUserRangeDescriptor3Replicas.RangeID, - Lease: &roachpb.Lease{Replica: recognizedLeaseHolder, Sequence: 1}, + RangeID: testUserRangeDescriptor3Replicas.RangeID, + Lease: &roachpb.Lease{Replica: recognizedLeaseHolder, Sequence: 1}, + RangeDesc: testUserRangeDescriptor3Replicas, }, expLeaseholder: &recognizedLeaseHolder, expLease: true, @@ -600,6 +605,7 @@ func TestRetryOnNotLeaseHolderError(t *testing.T) { nlhe: roachpb.NotLeaseHolderError{ RangeID: testUserRangeDescriptor3Replicas.RangeID, LeaseHolder: &recognizedLeaseHolder, + RangeDesc: testUserRangeDescriptor3Replicas, }, expLeaseholder: &recognizedLeaseHolder, expLease: false, @@ -607,16 +613,18 @@ func TestRetryOnNotLeaseHolderError(t *testing.T) { { name: "leaseholder not in desc", nlhe: roachpb.NotLeaseHolderError{ - RangeID: testUserRangeDescriptor3Replicas.RangeID, - Lease: &roachpb.Lease{Replica: unrecognizedLeaseHolder, Sequence: 2}, + RangeID: testUserRangeDescriptor3Replicas.RangeID, + Lease: &roachpb.Lease{Replica: unrecognizedLeaseHolder, Sequence: 2}, + RangeDesc: testUserRangeDescriptor3Replicas, }, expLeaseholder: nil, }, { name: "leaseholder in desc with different type", nlhe: roachpb.NotLeaseHolderError{ - RangeID: testUserRangeDescriptor3Replicas.RangeID, - Lease: &roachpb.Lease{Replica: recognizedLeaseHolderIncoming, Sequence: 1}, + RangeID: testUserRangeDescriptor3Replicas.RangeID, + Lease: &roachpb.Lease{Replica: recognizedLeaseHolderIncoming, Sequence: 1}, + RangeDesc: testUserRangeDescriptor3Replicas, }, expLeaseholder: &recognizedLeaseHolderIncoming, expLease: true, @@ -624,7 +632,8 @@ func TestRetryOnNotLeaseHolderError(t *testing.T) { { name: "leaseholder unknown", nlhe: roachpb.NotLeaseHolderError{ - RangeID: testUserRangeDescriptor3Replicas.RangeID, + RangeID: testUserRangeDescriptor3Replicas.RangeID, + RangeDesc: testUserRangeDescriptor3Replicas, }, expLeaseholder: nil, }, @@ -4738,6 +4747,14 @@ func TestSendToReplicasSkipsStaleReplicas(t *testing.T) { NodeID: 3, Address: util.UnresolvedAddr{}, }, + { + NodeID: 4, + Address: util.UnresolvedAddr{}, + }, + { + NodeID: 5, + Address: util.UnresolvedAddr{}, + }, }, } var desc = roachpb.RangeDescriptor{ @@ -4748,13 +4765,38 @@ func TestSendToReplicasSkipsStaleReplicas(t *testing.T) { InternalReplicas: []roachpb.ReplicaDescriptor{ {NodeID: 1, StoreID: 1, ReplicaID: 1}, {NodeID: 2, StoreID: 2, ReplicaID: 2}, + {NodeID: 3, StoreID: 3, ReplicaID: 3}, + {NodeID: 5, StoreID: 5, ReplicaID: 5}, }, } - var updatedDesc = roachpb.RangeDescriptor{ + var learnerDesc = roachpb.RangeDescriptor{ + RangeID: roachpb.RangeID(1), + Generation: 1, + StartKey: roachpb.RKeyMin, + EndKey: roachpb.RKeyMax, + InternalReplicas: []roachpb.ReplicaDescriptor{ + {NodeID: 1, StoreID: 1, ReplicaID: 1}, + {NodeID: 2, StoreID: 2, ReplicaID: 2}, + // The would be leaseholder is a learner on this range descriptor. + {NodeID: 4, StoreID: 4, ReplicaID: 4, Type: roachpb.LEARNER}, + }, + } + var incompatibleDescriptor = roachpb.RangeDescriptor{ RangeID: roachpb.RangeID(1), Generation: 2, StartKey: roachpb.RKeyMin, EndKey: roachpb.RKeyMax, + InternalReplicas: []roachpb.ReplicaDescriptor{ + {NodeID: 1, StoreID: 1, ReplicaID: 1}, + {NodeID: 2, StoreID: 2, ReplicaID: 2}, + {NodeID: 5, StoreID: 5, ReplicaID: 5}, + }, + } + var compatibleDescriptor = roachpb.RangeDescriptor{ + RangeID: roachpb.RangeID(1), + Generation: 3, + StartKey: roachpb.RKeyMin, + EndKey: roachpb.RKeyMax, InternalReplicas: []roachpb.ReplicaDescriptor{ {NodeID: 1, StoreID: 1, ReplicaID: 1}, {NodeID: 4, StoreID: 4, ReplicaID: 4}, @@ -4763,115 +4805,156 @@ func TestSendToReplicasSkipsStaleReplicas(t *testing.T) { for _, tc := range []struct { name string - // updatedDesc, if not nil, is used to update the range cache in the middle - // of the first RPC. - updatedDesc *roachpb.RangeDescriptor + // initialDesc is the descriptor which the range cache starts off with. + initialDesc roachpb.RangeDescriptor + // updatedDesc is either returned as part of the NotLeaseHolderError or is + // used to update the range cache in the middle of the first RPC (depending + // on the flavour of the test). + updatedDesc roachpb.RangeDescriptor // expLeaseholder is the leaseholder that the cache is expected to be // populated with after the RPC. If 0, the cache is expected to not have an - // entry corresponding to the descriptor in question - i.e. we expect the - // descriptor to have been evicted. + // empty leaseholder. expLeaseholder roachpb.ReplicaID + // expCalls is the number of replicas that we expect to try before returning + // from sendToReplicas(). + expReplicasTried int }{ { - name: "no intervening update", + name: "incompatible descriptor", + initialDesc: desc, // In this test, the NotLeaseHolderError will point to a replica that's - // not part of the cached descriptor. The cached descriptor is going to be - // considered stale and evicted. - updatedDesc: nil, + // not part of the cached descriptor. We expect the call to + // sendToReplicas() to return early, without trying all replicas on the + // initial descriptor. + updatedDesc: incompatibleDescriptor, expLeaseholder: 0, + // We expect sendToReplicas() to try all replicas in the transport that + // are still part of the freshest range descriptor (replicas 1, 2 and 5). + expReplicasTried: 3, }, { - name: "intervening update", + name: "compatible descriptor", + initialDesc: desc, // In this test, the NotLeaseHolderError will point to a replica that's // part of the cached descriptor (at the time when the DistSender gets the // error). Thus, the cache entry will be updated with the lease. - updatedDesc: &updatedDesc, + updatedDesc: compatibleDescriptor, expLeaseholder: 4, + // Given the leaseholder is incompatible with the original descriptor + // (and thus the constructed transport), we expect the test to exit early + // without exhausting the entire transport. + expReplicasTried: 1, + }, + { + name: "learner descriptor - compatible", + initialDesc: learnerDesc, + // In this test, the NotLeaseHolderError will point to a replica that's + // part of the cached descriptor (at the time when the DistSender gets the + // error). Thus, the cache entry will be updated with the lease. + updatedDesc: compatibleDescriptor, + expLeaseholder: 4, + // Given the leaseholder was a learner on the original descriptor, and + // thus excluded from the transport, we expect the test to exit + // early without exhausting the entire transport. + // + // This serves as a regression test for the hazard described in + // https://github.com/cockroachdb/cockroach/issues/75742. + expReplicasTried: 1, }, } { t.Run(tc.name, func(t *testing.T) { - st := cluster.MakeTestingClusterSettings() - tr := tracing.NewTracer() - getRangeDescCacheSize := func() int64 { - return 1 << 20 - } - rc := rangecache.NewRangeCache(st, nil /* db */, getRangeDescCacheSize, stopper, tr) - rc.Insert(ctx, roachpb.RangeInfo{ - Desc: desc, - Lease: roachpb.Lease{ - Replica: roachpb.ReplicaDescriptor{ - NodeID: 1, StoreID: 1, ReplicaID: 1, - }, - }, - }) - ent, err := rc.Lookup(ctx, roachpb.RKeyMin) - require.NoError(t, err) - tok := rc.MakeEvictionToken(&ent) - - var called bool - transportFn := func(_ context.Context, ba roachpb.BatchRequest) (*roachpb.BatchResponse, error) { - // We don't expect more than one RPC because we return a lease pointing - // to a replica that's not in the descriptor that sendToReplicas() was - // originally called with. sendToReplicas() doesn't deal with that; it - // returns a sendError and expects that caller to retry. - if called { - return nil, errors.New("unexpected 2nd call") + testutils.RunTrueAndFalse(t, "concurrent-update", func(t *testing.T, concurrentUpdate bool) { + // concurrentUpdate, if true, means that the descriptor was updated in + // the range cache by a concurrent request. This is in contrast to the + // updated descriptor being returned as part of the NotLeaseHolderError + // itself. + + st := cluster.MakeTestingClusterSettings() + tr := tracing.NewTracer() + getRangeDescCacheSize := func() int64 { + return 1 << 20 } - called = true - nlhe := &roachpb.NotLeaseHolderError{ - RangeID: desc.RangeID, - LeaseHolder: &roachpb.ReplicaDescriptor{ - NodeID: 4, - StoreID: 4, - ReplicaID: 4, + rc := rangecache.NewRangeCache(st, nil /* db */, getRangeDescCacheSize, stopper, tr) + rc.Insert(ctx, roachpb.RangeInfo{ + Desc: tc.initialDesc, + Lease: roachpb.Lease{ + Replica: roachpb.ReplicaDescriptor{ + NodeID: 1, StoreID: 1, ReplicaID: 1, + }, }, - CustomMsg: "injected", - } - if tc.updatedDesc != nil { - rc.Insert(ctx, roachpb.RangeInfo{Desc: *tc.updatedDesc}) + }) + ent, err := rc.Lookup(ctx, roachpb.RKeyMin) + require.NoError(t, err) + tok := rc.MakeEvictionToken(&ent) + + numCalled := 0 + transportFn := func(_ context.Context, ba roachpb.BatchRequest) (*roachpb.BatchResponse, error) { + numCalled++ + nlhe := &roachpb.NotLeaseHolderError{ + RangeID: tc.initialDesc.RangeID, + Lease: &roachpb.Lease{ + Replica: roachpb.ReplicaDescriptor{ + NodeID: 4, + StoreID: 4, + ReplicaID: 4, + }, + Sequence: 1, // we don't want the lease to be speculative. + }, + CustomMsg: "injected", + } + if concurrentUpdate { + rc.Insert(ctx, roachpb.RangeInfo{Desc: tc.updatedDesc}) + // Ensure the descriptor on the NLHE is compatible but older than + // what is in the cache. + nlhe.RangeDesc = tc.initialDesc + } else { + nlhe.RangeDesc = tc.updatedDesc + } + br := &roachpb.BatchResponse{} + br.Error = roachpb.NewError(nlhe) + return br, nil } - br := &roachpb.BatchResponse{} - br.Error = roachpb.NewError(nlhe) - return br, nil - } - cfg := DistSenderConfig{ - AmbientCtx: log.MakeTestingAmbientContext(tr), - Clock: clock, - NodeDescs: ns, - RPCContext: rpcContext, - RangeDescriptorDB: MockRangeDescriptorDB(func(key roachpb.RKey, reverse bool) ( - []roachpb.RangeDescriptor, []roachpb.RangeDescriptor, error, - ) { - // These tests only deal with the low-level sendToReplicas(). Nobody - // should be reading descriptor from the database, but the DistSender - // insists on having a non-nil one. - return nil, nil, errors.New("range desc db unexpectedly used") - }), - TestingKnobs: ClientTestingKnobs{ - TransportFactory: adaptSimpleTransport(transportFn), - }, - Settings: cluster.MakeTestingClusterSettings(), - } + cfg := DistSenderConfig{ + AmbientCtx: log.MakeTestingAmbientContext(tr), + Clock: clock, + NodeDescs: ns, + RPCContext: rpcContext, + RangeDescriptorDB: MockRangeDescriptorDB(func(key roachpb.RKey, reverse bool) ( + []roachpb.RangeDescriptor, []roachpb.RangeDescriptor, error, + ) { + // These tests only deal with the low-level sendToReplicas(). Nobody + // should be reading descriptor from the database, but the DistSender + // insists on having a non-nil one. + return nil, nil, errors.New("range desc db unexpectedly used") + }), + TestingKnobs: ClientTestingKnobs{ + TransportFactory: adaptSimpleTransport(transportFn), + }, + Settings: cluster.MakeTestingClusterSettings(), + } - ds := NewDistSender(cfg) + ds := NewDistSender(cfg) - var ba roachpb.BatchRequest - get := &roachpb.GetRequest{} - get.Key = roachpb.Key("a") - ba.Add(get) - _, err = ds.sendToReplicas(ctx, ba, tok, false /* withCommit */) - require.IsType(t, sendError{}, err) - require.Regexp(t, "NotLeaseHolderError", err) - cached := rc.GetCached(ctx, desc.StartKey, false /* inverted */) - if tc.expLeaseholder == 0 { - // Check that the descriptor was removed from the cache. - require.Nil(t, cached) - } else { + var ba roachpb.BatchRequest + get := &roachpb.GetRequest{} + get.Key = roachpb.Key("a") + ba.Add(get) + _, err = ds.sendToReplicas(ctx, ba, tok, false /* withCommit */) + require.IsType(t, sendError{}, err) + require.Regexp(t, "NotLeaseHolderError", err) + cached := rc.GetCached(ctx, tc.initialDesc.StartKey, false /* inverted */) require.NotNil(t, cached) - require.NotNil(t, cached.Leaseholder()) - require.Equal(t, tc.expLeaseholder, cached.Leaseholder().ReplicaID) - } + require.Equal(t, tc.updatedDesc, *cached.Desc()) + require.Equal(t, tc.expReplicasTried, numCalled) + if tc.expLeaseholder == 0 { + // Check that the leaseholder is cleared out. + require.Nil(t, cached.Leaseholder()) + } else { + require.NotNil(t, cached.Leaseholder()) + require.Equal(t, tc.expLeaseholder, cached.Leaseholder().ReplicaID) + } + }) }) } } @@ -5065,3 +5148,121 @@ func TestDistSenderRPCMetrics(t *testing.T) { require.Equal(t, ds.metrics.ErrCounts[roachpb.NotLeaseHolderErrType].Count(), int64(1)) require.Equal(t, ds.metrics.ErrCounts[roachpb.ConditionFailedErrType].Count(), int64(1)) } + +// TestDistSenderNLHEFromUninitializedReplicaDoesNotCauseUnboundedBackoff +// ensures that a NLHE from an uninitialized replica, which points to a replica +// that isn't part of the range, doesn't result in the dist sender getting +// caught in an unbounded backoff. See +// https://github.com/cockroachdb/cockroach/issues/82802 for more details about +// the hazard. +func TestDistSenderNLHEFromUninitializedReplicaDoesNotCauseUnboundedBackoff(t *testing.T) { + defer leaktest.AfterTest(t)() + ctx := context.Background() + stopper := stop.NewStopper() + defer stopper.Stop(ctx) + + // We'll set things up such that the first replica on the range descriptor of + // the client has an uninitialized replica. We'll mimic this by returning an + // empty range descriptor as part of the NotLeaseHolderError it returns. + // Moreover, the error will point to a replica that isn't part of the client's + // range descriptor. We expect the client to disregard this NLHE and simply + // reroute to the next replica. + + clock := hlc.NewClockWithSystemTimeSource(time.Nanosecond /* maxOffset */) + rpcContext := rpc.NewInsecureTestingContext(ctx, clock, stopper) + ns := &mockNodeStore{nodes: []roachpb.NodeDescriptor{ + {NodeID: 1, Address: util.UnresolvedAddr{}}, + {NodeID: 2, Address: util.UnresolvedAddr{}}, + {NodeID: 3, Address: util.UnresolvedAddr{}}, + {NodeID: 4, Address: util.UnresolvedAddr{}}, + }} + + // Actual view of the range (descriptor + lease). The client doesn't have + // any knowledge about the lease, so it routes its request to the first + // replica on the range descriptor. + var desc = roachpb.RangeDescriptor{ + RangeID: roachpb.RangeID(1), + Generation: 1, + StartKey: roachpb.RKeyMin, + EndKey: roachpb.RKeyMax, + InternalReplicas: []roachpb.ReplicaDescriptor{ + {NodeID: 1, StoreID: 1, ReplicaID: 1}, + {NodeID: 2, StoreID: 2, ReplicaID: 2}, + {NodeID: 3, StoreID: 3, ReplicaID: 3}, + {NodeID: 4, StoreID: 4, ReplicaID: 4}, + }, + } + leaseResp := roachpb.Lease{ + Replica: roachpb.ReplicaDescriptor{NodeID: 4, StoreID: 4, ReplicaID: 4}, + } + + call := 0 + var transportFn = func(_ context.Context, ba roachpb.BatchRequest) (*roachpb.BatchResponse, error) { + br := &roachpb.BatchResponse{} + switch call { + // First we return the replica on store 3 as the leaseholder. This ensures + // the range cache is seeded with some lease. + case 0: + expRepl := desc.Replicas().Descriptors()[0] + require.Equal(t, expRepl, ba.Replica) + br.Error = roachpb.NewError(&roachpb.NotLeaseHolderError{ + RangeDesc: roachpb.RangeDescriptor{}, + LeaseHolder: &roachpb.ReplicaDescriptor{NodeID: 5, StoreID: 5, ReplicaID: 5}, + }) + case 1: + // We expect the client to discard information from the NLHE above and + // instead just try the next replica. + expRepl := desc.Replicas().Descriptors()[1] + require.Equal(t, expRepl, ba.Replica) + br.Error = roachpb.NewError(&roachpb.NotLeaseHolderError{ + RangeDesc: desc, + Lease: &leaseResp, + }) + case 2: + // We expect the client to route to the leaseholder given it's known now. + expRepl := desc.Replicas().Descriptors()[3] + require.Equal(t, expRepl, ba.Replica) + br = ba.CreateReply() + default: + t.Fatal("unexpected") + } + call++ + return br, nil + } + + rangeLookups := 0 + cfg := DistSenderConfig{ + AmbientCtx: log.MakeTestingAmbientCtxWithNewTracer(), + Clock: clock, + NodeDescs: ns, + RPCContext: rpcContext, + RangeDescriptorDB: MockRangeDescriptorDB(func(key roachpb.RKey, reverse bool) ( + []roachpb.RangeDescriptor, []roachpb.RangeDescriptor, error, + ) { + switch rangeLookups { + case 0: + rangeLookups++ + return []roachpb.RangeDescriptor{desc}, nil, nil + default: + // This doesn't run on the test's goroutine. + panic("unexpected") + } + }), + TestingKnobs: ClientTestingKnobs{ + TransportFactory: adaptSimpleTransport(transportFn), + DontReorderReplicas: true, + }, + Settings: cluster.MakeTestingClusterSettings(), + } + + ds := NewDistSender(cfg) + var ba roachpb.BatchRequest + get := &roachpb.GetRequest{} + get.Key = roachpb.Key("a") + ba.Add(get) + + _, err := ds.Send(ctx, ba) + require.NoError(t, err.GoError()) + require.Equal(t, 3, call) + require.Equal(t, 1, rangeLookups) +} diff --git a/pkg/kv/kvclient/kvcoord/mocks_generated_test.go b/pkg/kv/kvclient/kvcoord/mocks_generated_test.go index 02323419cc74..efd75362a2db 100644 --- a/pkg/kv/kvclient/kvcoord/mocks_generated_test.go +++ b/pkg/kv/kvclient/kvcoord/mocks_generated_test.go @@ -51,9 +51,11 @@ func (mr *MockTransportMockRecorder) IsExhausted() *gomock.Call { } // MoveToFront mocks base method. -func (m *MockTransport) MoveToFront(arg0 roachpb.ReplicaDescriptor) { +func (m *MockTransport) MoveToFront(arg0 roachpb.ReplicaDescriptor) bool { m.ctrl.T.Helper() - m.ctrl.Call(m, "MoveToFront", arg0) + ret := m.ctrl.Call(m, "MoveToFront", arg0) + ret0, _ := ret[0].(bool) + return ret0 } // MoveToFront indicates an expected call of MoveToFront. diff --git a/pkg/kv/kvclient/kvcoord/send_test.go b/pkg/kv/kvclient/kvcoord/send_test.go index 87d392e399cc..bcc875850b08 100644 --- a/pkg/kv/kvclient/kvcoord/send_test.go +++ b/pkg/kv/kvclient/kvcoord/send_test.go @@ -178,7 +178,8 @@ func (f *firstNErrorTransport) SkipReplica() { panic("SkipReplica not supported") } -func (*firstNErrorTransport) MoveToFront(roachpb.ReplicaDescriptor) { +func (*firstNErrorTransport) MoveToFront(roachpb.ReplicaDescriptor) bool { + return true } // TestComplexScenarios verifies various complex success/failure scenarios by diff --git a/pkg/kv/kvclient/kvcoord/transport.go b/pkg/kv/kvclient/kvcoord/transport.go index 5663f42c50f2..68ea2b7ead00 100644 --- a/pkg/kv/kvclient/kvcoord/transport.go +++ b/pkg/kv/kvclient/kvcoord/transport.go @@ -81,9 +81,10 @@ type Transport interface { // MoveToFront locates the specified replica and moves it to the // front of the ordering of replicas to try. If the replica has - // already been tried, it will be retried. If the specified replica - // can't be found, this is a noop. - MoveToFront(roachpb.ReplicaDescriptor) + // already been tried, it will be retried. Returns false if the specified + // replica can't be found and thus can't be moved to the front of the + // transport. + MoveToFront(roachpb.ReplicaDescriptor) bool // Release releases any resources held by this Transport. Release() @@ -254,7 +255,7 @@ func (gt *grpcTransport) SkipReplica() { gt.nextReplicaIdx++ } -func (gt *grpcTransport) MoveToFront(replica roachpb.ReplicaDescriptor) { +func (gt *grpcTransport) MoveToFront(replica roachpb.ReplicaDescriptor) bool { for i := range gt.replicas { if gt.replicas[i].IsSame(replica) { // If we've already processed the replica, decrement the current @@ -264,9 +265,10 @@ func (gt *grpcTransport) MoveToFront(replica roachpb.ReplicaDescriptor) { } // Swap the client representing this replica to the front. gt.replicas[i], gt.replicas[gt.nextReplicaIdx] = gt.replicas[gt.nextReplicaIdx], gt.replicas[i] - return + return true } } + return false } // splitHealthy splits the grpcTransport's replica slice into healthy replica @@ -377,7 +379,8 @@ func (s *senderTransport) SkipReplica() { s.called = true } -func (s *senderTransport) MoveToFront(replica roachpb.ReplicaDescriptor) { +func (s *senderTransport) MoveToFront(replica roachpb.ReplicaDescriptor) bool { + return true } func (s *senderTransport) Release() {} diff --git a/pkg/kv/kvclient/rangecache/range_cache.go b/pkg/kv/kvclient/rangecache/range_cache.go index 988f0c00f059..0b88ada953f7 100644 --- a/pkg/kv/kvclient/rangecache/range_cache.go +++ b/pkg/kv/kvclient/rangecache/range_cache.go @@ -330,6 +330,15 @@ func (et EvictionToken) Leaseholder() *roachpb.ReplicaDescriptor { return &et.lease.Replica } +// Lease returns the cached lease. If the cache didn't have any lease +// information, returns nil. The result is considered immutable. +func (et EvictionToken) Lease() *roachpb.Lease { + if !et.Valid() { + return nil + } + return et.lease +} + // LeaseSeq returns the sequence of the cached lease. If no lease is cached, or // the cached lease is speculative, 0 is returned. func (et EvictionToken) LeaseSeq() roachpb.LeaseSequence { @@ -369,38 +378,38 @@ func (et *EvictionToken) syncRLocked( return true, cachedEntry, rawEntry } -// UpdateLease updates the leaseholder for the token's cache entry to the -// specified lease, and returns an updated EvictionToken, tied to the new cache -// entry. +// SyncTokenAndMaybeUpdateCache acts as a synchronization point between the +// caller and the RangeCache. It updates the EvictionToken with fresher +// information in case the EvictionToken was no longer up to date with the +// cache entry from whence it came. The leaseholder and range descriptor for the +// token's cache entry are updated to the specified lease/range descriptor if +// they are fresher than what the cache contains (which is reflected in the +// EvictionToken itself). // -// The bool retval is true if the requested update was performed (i.e. the -// passed-in lease was compatible with the descriptor and more recent than the -// cached lease). +// The returned bool `updatedLeaseholder` is true if the leaseholder was updated +// in the cache (i.e the passed-in lease was more recent than the cached lease). // -// UpdateLease also acts as a synchronization point between the caller and the -// RangeCache. In the spirit of a Compare-And-Swap operation (but -// unfortunately not quite as simple), it returns updated information (besides -// the lease) from the cache in case the EvictionToken was no longer up to date -// with the cache entry from whence it came. +// The updated token is guaranteed to contain a descriptor compatible with the +// original one (same range id and key span). If the cache no longer contains +// an entry for the start key or the range boundaries have changed, the token +// will be invalidated. The caller should take an invalidated token to mean that +// the information it was working with is too stale to be useful, and it should +// use a range iterator again to get an updated cache entry. // -// The updated token might have a newer descriptor than before and/or a newer -// lease than the one passed-in - in case the cache already had a more recent -// entry. The returned entry has a descriptor compatible to the original one -// (same range id and key span). +// It's legal to pass in a lease with a zero Sequence; it will be treated as a +// speculative lease and considered newer[1] than any existing lease (and then +// in-turn will be overwritten by any subsequent update). // -// If the passed-in lease is incompatible with the cached descriptor (i.e. the -// leaseholder is not a replica in the cached descriptor), then the existing -// entry is evicted and an invalid token is returned. The caller should take an -// invalid returned token to mean that the information it was working with is -// too stale to be useful, and it should use a range iterator again to get an -// updated cache entry. +// [1]: As long as the associated range descriptor is not older than what's +// cached. // -// It's legal to pass in a lease with a zero Sequence; it will be treated as a -// speculative lease and considered newer than any existing lease (and then in -// turn will be overridden by any subsequent update). -func (et *EvictionToken) UpdateLease( - ctx context.Context, l *roachpb.Lease, descGeneration roachpb.RangeGeneration, -) bool { +// TODO(arul): Instead of returning updatedLeaseholder all the way back to the +// DistSender and then using its value to determine whether we need to backoff, +// we should instead check if we're retrying the same replica. This will allow +// us to eschew plumbing this state back up to the caller. +func (et *EvictionToken) SyncTokenAndMaybeUpdateCache( + ctx context.Context, l *roachpb.Lease, rangeDesc *roachpb.RangeDescriptor, +) (updatedLeaseholder bool) { rdc := et.rdc rdc.rangeCache.Lock() defer rdc.rangeCache.Unlock() @@ -409,32 +418,59 @@ func (et *EvictionToken) UpdateLease( if !stillValid { return false } - ok, newEntry := cachedEntry.updateLease(l, descGeneration) - if !ok { + + // Check if the supplied range descriptor is compatible with the one in the + // cache. If it isn't, and the supplied range descriptor is newer than what's + // in the cache, we simply evict the old descriptor and add the new + // descriptor/lease pair. On the other hand, if the supplied range descriptor + // is older, we can simply return early. + if !descsCompatible(rangeDesc, et.Desc()) { + if rangeDesc.Generation < et.desc.Generation { + return false + } + // Newer descriptor. + ri := roachpb.RangeInfo{ + Desc: *rangeDesc, + Lease: *l, + ClosedTimestampPolicy: et.closedts, + } + et.evictAndReplaceLocked(ctx, ri) return false } - if newEntry != nil { - et.desc = newEntry.Desc() - et.lease = newEntry.leaseEvenIfSpeculative() - } else { - // newEntry == nil means the lease is not compatible with the descriptor. - et.clear() + + updated, updatedLeaseholder, newEntry := cachedEntry.maybeUpdate(ctx, l, rangeDesc) + if !updated { + // The cachedEntry wasn't updated; no need to replace it with newEntry in + // the RangeCache. + return false } rdc.swapEntryLocked(ctx, rawEntry, newEntry) - return newEntry != nil + + // Finish syncing the eviction token by updating its fields using the freshest + // range descriptor/lease information available in the RangeCache. + et.desc = newEntry.Desc() + et.lease = newEntry.leaseEvenIfSpeculative() + return updatedLeaseholder } -// UpdateLeaseholder is like UpdateLease(), but it only takes a leaseholder, not -// a full lease. This is called when a likely leaseholder is known, but not a -// full lease. The lease we'll insert into the cache will be considered -// "speculative". -func (et *EvictionToken) UpdateLeaseholder( - ctx context.Context, lh roachpb.ReplicaDescriptor, descGeneration roachpb.RangeGeneration, -) { +// SyncTokenAndMaybeUpdateCacheWithSpeculativeLease is like +// SyncTokenAndMaybeUpdateCache(), but it only takes a leaseholder, +// not a full lease. This is called when the likely leaseholder is known, but a +// full lease isn't. +// +// This method takes into account whether the speculative lease is worth paying +// attention to -- specifically, we disregard speculative leases from replicas +// that have an older view of the world (i.e, their range descriptor is older +// than what was already in the cache). Otherwise, the likely leaseholder is +// presumed to be newer than anything already in the cache. The boolean retval +// indicates if the speculative lease was indeed inserted into the cache. +func (et *EvictionToken) SyncTokenAndMaybeUpdateCacheWithSpeculativeLease( + ctx context.Context, lh roachpb.ReplicaDescriptor, rangeDesc *roachpb.RangeDescriptor, +) bool { // Notice that we don't initialize Lease.Sequence, which will make // entry.LeaseSpeculative() return true. l := &roachpb.Lease{Replica: lh} - et.UpdateLease(ctx, l, descGeneration) + return et.SyncTokenAndMaybeUpdateCache(ctx, l, rangeDesc) } // EvictLease evicts information about the current lease from the cache, if the @@ -445,15 +481,15 @@ func (et *EvictionToken) UpdateLeaseholder( // have a problem with, not a particular lease (i.e. we want to evict even a // newer lease, but with the same leaseholder). // -// Similarly to UpdateLease(), EvictLease() acts as a synchronization point -// between the caller and the RangeCache. The caller might get an -// updated token (besides the lease). Note that the updated token might have a -// newer descriptor than before and/or still have a lease in it - in case the -// cache already had a more recent entry. The updated descriptor is compatible -// (same range id and key span) to the original one. The token is invalidated if -// the cache has a more recent entry, but the current descriptor is -// incompatible. Callers should interpret such an update as a signal that they -// should use a range iterator again to get updated ranges. +// Similarly to SyncTokenAndMaybeUpdateCache(), EvictLease() acts as a +// synchronization point between the caller and the RangeCache. The caller might +// get an updated token (besides the lease). Note that the updated token might +// have a newer descriptor than before and/or still have a lease in it - in case +// the cache already had a more recent entry. The updated descriptor is +// compatible (same range id and key span) to the original one. The token is +// invalidated if the cache has a more recent entry, but the current descriptor +// is incompatible. Callers should interpret such an update as a signal that +// they should use a range iterator again to get updated ranges. func (et *EvictionToken) EvictLease(ctx context.Context) { et.rdc.rangeCache.Lock() defer et.rdc.rangeCache.Unlock() @@ -500,6 +536,16 @@ func (et *EvictionToken) EvictAndReplace(ctx context.Context, newDescs ...roachp et.rdc.rangeCache.Lock() defer et.rdc.rangeCache.Unlock() + et.evictAndReplaceLocked(ctx, newDescs...) +} + +// evictAndReplaceLocked is like EvictAndReplace except it assumes that the +// caller holds a write lock on rdc.rangeCache. +func (et *EvictionToken) evictAndReplaceLocked(ctx context.Context, newDescs ...roachpb.RangeInfo) { + if !et.Valid() { + panic("trying to evict an invalid token") + } + // Evict unless the cache has something newer. Regardless of what the cache // has, we'll still attempt to insert newDescs (if any). evicted := et.rdc.evictDescLocked(ctx, et.Desc()) @@ -1142,8 +1188,9 @@ type CacheEntry struct { // Lease has info on the range's lease. It can be Empty() if no lease // information is known. When a lease is known, it is guaranteed that the // lease comes from Desc's range id (i.e. we'll never put a lease from another - // range in here). This allows UpdateLease() to use Lease.Sequence to compare - // leases. Moreover, the lease will correspond to one of the replicas in Desc. + // range in here). This allows SyncTokenAndMaybeUpdateCache() to use + // Lease.Sequence to compare leases. Moreover, the lease will correspond to + // one of the replicas in Desc. lease roachpb.Lease // closedts indicates the range's closed timestamp policy. closedts roachpb.RangeClosedTimestampPolicy @@ -1327,74 +1374,99 @@ func compareEntryLeases(a, b *CacheEntry) int { return 0 } -// updateLease returns a new CacheEntry with the receiver's descriptor and -// a new lease. The updated retval indicates whether the passed-in lease appears -// to be newer than the lease the entry had before. If updated is returned true, -// the caller should evict the existing entry (the receiver) and replace it with -// newEntry. (true, nil) can be returned meaning that the existing entry should -// be evicted, but there's no replacement that this function can provide; this -// happens when the passed-in lease indicates a leaseholder that's not part of -// the entry's descriptor. The descriptor must be really stale, and the caller -// should read a new version. +// maybeUpdate returns a new CacheEntry which contains the freshest lease/range +// descriptor by comparing the receiver's fields to the passed-in parameters. // -// If updated=false is returned, then newEntry will be the same as the receiver. -// This means that the passed-in lease is older than the lease already in the -// entry. +// The updated retval indicates if either the passed-in lease or the range +// descriptor or both appear to be newer than what the entry had before. If +// updated is returned true, the caller should evict the existing entry +// (the receiver) and replace it with the newEntry. // -// If the new leaseholder is not a replica in the descriptor, and the error is -// coming from a replica with range descriptor generation at least as high as -// the cache's, we deduce the lease information to be more recent than the -// entry's descriptor, and we return true, nil. The caller should evict the -// receiver from the cache, but it'll have to do extra work to figure out what -// to insert instead. -func (e *CacheEntry) updateLease( - l *roachpb.Lease, descGeneration roachpb.RangeGeneration, -) (updated bool, newEntry *CacheEntry) { - // If l is older than what the entry has (or the same), return early. - // - // This method handles speculative leases: a new lease with a sequence of 0 is - // presumed to be newer than anything, and an existing lease with a sequence - // of 0 is presumed to be older than anything. - // - // We handle the case of a lease with the sequence equal to the existing - // entry, but otherwise different. This results in the new lease updating the - // entry, because the existing lease might correspond to a proposed lease that - // a replica returned speculatively while a lease acquisition was in progress. - if l.Sequence != 0 && e.lease.Sequence != 0 && l.Sequence < e.lease.Sequence { - return false, e +// If updated=false is returned, then newEntry will be the same as the receiver +// This means that both the passed-in lease and the range descriptor are the +// same or older than the lease and range descriptor already in the entry. +// +// The updatedLease retval indicates whether the passed-in lease appears to be +// newer than the lease the entry had before. If updatedLease is returned true, +// updated will be true as well, so the caller should perform the same eviction +// and replacement described above. +// +// If the freshest combination of lease/range descriptor pair is incompatible +// (i.e the range descriptor doesn't contain the leaseholder replica), the +// returned cache entry will have an empty lease field. +// +// It's expected that the supplied rangeDesc is compatible with the descriptor +// on the cache entry. +func (e *CacheEntry) maybeUpdate( + ctx context.Context, l *roachpb.Lease, rangeDesc *roachpb.RangeDescriptor, +) (updated, updatedLease bool, newEntry *CacheEntry) { + if !descsCompatible(e.Desc(), rangeDesc) { + log.Fatalf(ctx, "attempting to update by comparing non-compatible descs: %s vs %s", + e.Desc(), rangeDesc) + } + + newEntry = &CacheEntry{ + lease: e.lease, + desc: e.desc, + closedts: e.closedts, } - if l.Equal(e.Lease()) { - return false, e - } + updatedLease = false + updatedDesc := false - // If the lease is incompatible with the cached descriptor and the error is - // coming from a replica that has a non-stale descriptor, the cached - // descriptor must be stale and the RangeCacheEntry needs to be evicted. - _, ok := e.desc.GetReplicaDescriptorByID(l.Replica.ReplicaID) + // First, we handle the lease. If l is older than what the entry has (or the + // same), there's nothing to update. + // + // This method handles speculative leases: a new lease with a sequence of 0 is + // presumed to be newer than anything if the replica it's coming from doesn't + // have an outdated view of the world (i.e does not have an older range + // descriptor than the one cached on the client); an existing lease with a + // sequence number of 0 is presumed to be older than anything and will be + // replaced by the supplied lease if the associated range descriptor is + // non-stale. + // + // Lastly, if the cached lease is empty, it will be updated with the supplied + // lease regardless of the associated range descriptor's generation. + if (l.Sequence != 0 && e.lease.Sequence != 0 && l.Sequence > e.lease.Sequence) || + ((l.Sequence == 0 || e.lease.Sequence == 0) && rangeDesc.Generation >= e.desc.Generation) || + e.lease.Empty() { + newEntry.lease = *l + updatedLease = true + } + + // We only want to use the supplied rangeDesc if its generation indicates it's + // strictly newer than what the was in the cache entry. + if e.desc.Generation < rangeDesc.Generation { + newEntry.desc = *rangeDesc + updatedDesc = true + } + + // Lastly, we want to check if the leaseholder is indeed a replica + // (in some form) on the descriptor. The range cache doesn't like when this + // isn't the case and doesn't allow us to insert an incompatible + // leaseholder/range descriptor pair. Given we only ever insert incrementally + // fresher range descriptors in the cache, we choose to empty out the lease + // field and insert the range descriptor. Going through each of the replicas + // on the range descriptor further up the stack should provide us with a + // compatible and actionable state of the world. + // + // TODO(arul): While having this safeguard, logging, and not fatal-ing further + // down seems like the right thing to do, do we expect this to happen in the + // wild? + _, ok := newEntry.desc.GetReplicaDescriptorByID(newEntry.lease.Replica.ReplicaID) if !ok { - // If the error is coming from a replica that has a stale range descriptor, - // we cannot trigger a cache eviction since this means we've rebalanced the - // old leaseholder away. If we were to evict here, we'd keep evicting until - // this replica applied the new lease. Not updating the cache here means - // that we'll end up trying all voting replicas until we either hit the new - // leaseholder or hit a replica that has accurate knowledge of the - // leaseholder. - if descGeneration != 0 && descGeneration < e.desc.Generation { - return false, nil - } - return true, nil - } - - // TODO(andrei): If the leaseholder is present, but the descriptor lists the - // replica as a learner, this is a sign of a stale descriptor. I'm not sure - // what to do about it, though. - - return true, &CacheEntry{ - desc: e.desc, - lease: *l, - closedts: e.closedts, - } + log.VEventf( + ctx, + 2, + "incompatible leaseholder id: %d/descriptor %v pair; eliding lease update to the cache", + newEntry.lease.Replica.ReplicaID, + newEntry.Desc(), + ) + newEntry.lease = roachpb.Lease{} + updatedLease = false + } + + return updatedLease || updatedDesc, updatedLease, newEntry } func (e *CacheEntry) evictLeaseholder( diff --git a/pkg/kv/kvclient/rangecache/range_cache_test.go b/pkg/kv/kvclient/rangecache/range_cache_test.go index 1bd71eec9fed..1ce836ae9b9b 100644 --- a/pkg/kv/kvclient/rangecache/range_cache_test.go +++ b/pkg/kv/kvclient/rangecache/range_cache_test.go @@ -1543,7 +1543,13 @@ func TestRangeCacheEvictAndReplace(t *testing.T) { require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, tok.ClosedTimestampPolicy()) } -func TestRangeCacheUpdateLease(t *testing.T) { +// TestRangeCacheSyncTokenAndMaybeUpdateCache tests +// RangeCacheSyncTokenAndMaybeUpdateCache() by ensuring the cache entry returned +// contains the freshest (lease, range desc) combination given the arguments +// supplied and what exists in the cache. Additionally, we also test that the +// method only updates the cache with speculative leases if the accompanying +// range descriptor is at-least as old as what is contained in the cache. +func TestRangeCacheSyncTokenAndMaybeUpdateCache(t *testing.T) { defer leaktest.AfterTest(t)() ctx := context.Background() @@ -1562,21 +1568,23 @@ func TestRangeCacheUpdateLease(t *testing.T) { StoreID: 3, ReplicaID: 3, } - repNonMember := roachpb.ReplicaDescriptor{ - NodeID: 4, - StoreID: 4, - ReplicaID: 4, - } - staleRangeGeneration := roachpb.RangeGeneration(2) - nonStaleRangeGeneration := roachpb.RangeGeneration(3) + currentGeneration := roachpb.RangeGeneration(3) + staleRangeDescriptor := roachpb.RangeDescriptor{ + StartKey: roachpb.RKeyMin, + EndKey: roachpb.RKeyMax, + InternalReplicas: []roachpb.ReplicaDescriptor{ + rep1, rep3, + }, + Generation: currentGeneration - 1, + } desc1 := roachpb.RangeDescriptor{ StartKey: roachpb.RKeyMin, EndKey: roachpb.RKeyMax, InternalReplicas: []roachpb.ReplicaDescriptor{ rep1, rep2, }, - Generation: nonStaleRangeGeneration, + Generation: currentGeneration, } desc2 := roachpb.RangeDescriptor{ StartKey: roachpb.RKeyMin, @@ -1584,7 +1592,7 @@ func TestRangeCacheUpdateLease(t *testing.T) { InternalReplicas: []roachpb.ReplicaDescriptor{ rep2, rep3, }, - Generation: nonStaleRangeGeneration + 1, + Generation: currentGeneration + 1, } desc3 := roachpb.RangeDescriptor{ StartKey: roachpb.RKeyMin, @@ -1592,7 +1600,16 @@ func TestRangeCacheUpdateLease(t *testing.T) { InternalReplicas: []roachpb.ReplicaDescriptor{ rep1, rep2, }, - Generation: nonStaleRangeGeneration + 2, + Generation: currentGeneration + 2, + } + // Incompatible key bounds/range ID with other descriptors. + incompatibleDescriptor := roachpb.RangeDescriptor{ + RangeID: 1, + StartKey: roachpb.RKey(keys.TableDataMin), + EndKey: roachpb.RKey(keys.TableDataMax), + InternalReplicas: []roachpb.ReplicaDescriptor{ + rep1, rep2, + }, } startKey := desc1.StartKey @@ -1602,109 +1619,325 @@ func TestRangeCacheUpdateLease(t *testing.T) { defer stopper.Stop(ctx) cache := NewRangeCache(st, nil, staticSize(2<<10), stopper, tr) - cache.Insert(ctx, roachpb.RangeInfo{ - Desc: desc1, - Lease: roachpb.Lease{}, - ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, - }) - - // Check that initially the cache has an empty lease. Then, we'll UpdateLease(). - tok, err := cache.LookupWithEvictionToken( - ctx, desc1.StartKey, EvictionToken{}, false /* useReverseScan */) - require.NoError(t, err) - require.Equal(t, desc1, *tok.Desc()) - require.Nil(t, tok.Leaseholder()) - require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, tok.ClosedTimestampPolicy()) - - l := &roachpb.Lease{ - Replica: rep1, - Sequence: 1, - } - oldTok := tok - ok := tok.UpdateLease(ctx, l, 0 /* descGeneration */) - require.True(t, ok) - require.Equal(t, oldTok.Desc(), tok.Desc()) - require.Equal(t, &l.Replica, tok.Leaseholder()) - require.Equal(t, oldTok.ClosedTimestampPolicy(), tok.ClosedTimestampPolicy()) - ri := cache.GetCached(ctx, startKey, false /* inverted */) - require.NotNil(t, ri) - require.Equal(t, desc1, *ri.Desc()) - require.Equal(t, rep1, ri.Lease().Replica) - require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, ri.ClosedTimestampPolicy()) - - oldTok = tok - tok.EvictLease(ctx) - require.Equal(t, oldTok.Desc(), tok.Desc()) - require.Nil(t, tok.Leaseholder()) - require.Equal(t, oldTok.ClosedTimestampPolicy(), tok.ClosedTimestampPolicy()) - ri = cache.GetCached(ctx, startKey, false /* inverted */) - require.NotNil(t, ri) - require.Equal(t, desc1, *ri.Desc()) - require.True(t, ri.lease.Empty()) - require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, ri.ClosedTimestampPolicy()) - - // Check that trying to update the lease to a non-member replica results in - // the entry's eviction and the token's invalidation if the descriptor - // generation in the error is not older than the cached descriptor generation. - l = &roachpb.Lease{ - Replica: repNonMember, - Sequence: 2, + testCases := []struct { + name string + testFn func(*testing.T, *RangeCache) + }{ + { + name: "basic", + testFn: func(t *testing.T, cache *RangeCache) { + cache.Insert(ctx, roachpb.RangeInfo{ + Desc: desc1, + Lease: roachpb.Lease{}, + ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, + }) + + // Check that initially the cache has an empty lease. Then, we'll + // call SyncTokenAndMaybeUpdateCache(). + tok, err := cache.LookupWithEvictionToken( + ctx, startKey, EvictionToken{}, false /* useReverseScan */) + require.NoError(t, err) + require.Equal(t, desc1, *tok.Desc()) + require.Nil(t, tok.Leaseholder()) + require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, tok.ClosedTimestampPolicy()) + + l := &roachpb.Lease{ + Replica: rep1, + Sequence: 1, + } + oldTok := tok + updatedLeaseholder := tok.SyncTokenAndMaybeUpdateCache(ctx, l, &desc1) + require.True(t, updatedLeaseholder) + require.Equal(t, oldTok.Desc(), tok.Desc()) + require.Equal(t, &l.Replica, tok.Leaseholder()) + require.Equal(t, oldTok.ClosedTimestampPolicy(), tok.ClosedTimestampPolicy()) + ri := cache.GetCached(ctx, startKey, false /* inverted */) + require.NotNil(t, ri) + require.Equal(t, desc1, *ri.Desc()) + require.Equal(t, rep1, ri.Lease().Replica) + require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, ri.ClosedTimestampPolicy()) + + // Ensure evicting the lease doesn't remove the closed timestamp + // policy/desc. + oldTok = tok + tok.EvictLease(ctx) + require.Equal(t, oldTok.Desc(), tok.Desc()) + require.Nil(t, tok.Leaseholder()) + require.Equal(t, oldTok.ClosedTimestampPolicy(), tok.ClosedTimestampPolicy()) + ri = cache.GetCached(ctx, startKey, false /* inverted */) + require.NotNil(t, ri) + require.Equal(t, desc1, *ri.Desc()) + require.True(t, ri.lease.Empty()) + require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, ri.ClosedTimestampPolicy()) + }, + }, + { + name: "sync newer descriptor", + testFn: func(t *testing.T, cache *RangeCache) { + // Check that updating the lease while the cache has a newer descriptor + // updates the token to the newer descriptor. + + cache.Insert(ctx, roachpb.RangeInfo{ + Desc: desc1, + Lease: roachpb.Lease{}, + ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, + }) + tok, err := cache.LookupWithEvictionToken( + ctx, startKey, EvictionToken{}, false /* useReverseScan */) + require.NoError(t, err) + + // Update the cache. + cache.Insert(ctx, roachpb.RangeInfo{ + Desc: desc2, + Lease: roachpb.Lease{}, + }) + updatedLeaseholder := tok.SyncTokenAndMaybeUpdateCache( + ctx, &roachpb.Lease{Replica: rep2, Sequence: 3}, &staleRangeDescriptor, + ) + require.True(t, updatedLeaseholder) + require.NotNil(t, tok) + require.Equal(t, &desc2, tok.Desc()) + require.Equal(t, &rep2, tok.Leaseholder()) + require.Equal(t, tok.lease.Replica, rep2) + require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, tok.ClosedTimestampPolicy()) + }, + }, + { + name: "sync freshest descriptor", + testFn: func(t *testing.T, cache *RangeCache) { + // Check that trying to update the descriptor with something fresher + // than what is on the token but stale in comparison to what's contained + // in the cache behaves correctly. Specifically, the (freshest) + // descriptor from the cache should be on the token. + cache.Insert(ctx, roachpb.RangeInfo{ + Desc: desc1, + Lease: roachpb.Lease{}, + ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, + }) + tok, err := cache.LookupWithEvictionToken( + ctx, startKey, EvictionToken{}, false /* useReverseScan */) + require.NoError(t, err) + + l := roachpb.Lease{ + Replica: rep2, + Sequence: 3, + } + // Update the cache. + cache.Insert(ctx, roachpb.RangeInfo{ + Desc: desc3, + Lease: l, + ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, + }) + updatedLeaseholder := tok.SyncTokenAndMaybeUpdateCache(ctx, &l, &desc2) + require.False(t, updatedLeaseholder) + require.NotNil(t, tok) + require.Equal(t, &desc3, tok.Desc()) + require.Equal(t, &rep2, tok.Leaseholder()) + require.Equal(t, tok.lease.Replica, rep2) + require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, tok.ClosedTimestampPolicy()) + }, + }, + { + name: "sync newer lease", + testFn: func(t *testing.T, cache *RangeCache) { + // Check that updating the descriptor while the cache has a newer lease + // updates the token to the newer lease. + cache.Insert(ctx, roachpb.RangeInfo{ + Desc: staleRangeDescriptor, + Lease: roachpb.Lease{ + Replica: rep3, + Sequence: 4, + }, + ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, + }) + tok, err := cache.LookupWithEvictionToken( + ctx, startKey, EvictionToken{}, false, /* useReverseScan */ + ) + require.NoError(t, err) + + updatedLeaseholder := tok.SyncTokenAndMaybeUpdateCache( + ctx, &roachpb.Lease{Replica: rep2, Sequence: 3}, &desc2, + ) + require.False(t, updatedLeaseholder) + require.NotNil(t, tok) + require.Equal(t, &desc2, tok.Desc()) + require.Equal(t, &rep3, tok.Leaseholder()) + require.Equal(t, tok.lease.Replica, rep3) + require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, tok.ClosedTimestampPolicy()) + }, + }, + { + name: "sync stale info", + testFn: func(t *testing.T, cache *RangeCache) { + // Check that trying to update the descriptor and lease while the token + // has newer versions of both is a no-op. + cache.Insert(ctx, roachpb.RangeInfo{ + Desc: desc2, + Lease: roachpb.Lease{ + Replica: rep3, + Sequence: 4, + }, + ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, + }) + tok, err := cache.LookupWithEvictionToken( + ctx, startKey, EvictionToken{}, false, /* useReverseScan */ + ) + require.NoError(t, err) + + updatedLeaseholder := tok.SyncTokenAndMaybeUpdateCache( + ctx, &roachpb.Lease{Replica: rep2, Sequence: 3}, &desc1, + ) + require.False(t, updatedLeaseholder) + require.NotNil(t, tok) + require.Equal(t, &desc2, tok.Desc()) + require.Equal(t, &rep3, tok.Leaseholder()) + require.Equal(t, tok.lease.Replica, rep3) + require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, tok.ClosedTimestampPolicy()) + }, + }, + { + name: "incompatible descriptor/lease", + testFn: func(t *testing.T, cache *RangeCache) { + // Check that trying to update the descriptor and lease such that the + // freshest lease and descriptor aren't compatible works as expected. In + // particular, the lease should be emptied out. + cache.Insert(ctx, roachpb.RangeInfo{ + Desc: desc2, + Lease: roachpb.Lease{ + Replica: rep3, + Sequence: 4, + }, + ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, + }) + tok, err := cache.LookupWithEvictionToken( + ctx, startKey, EvictionToken{}, false, /* useReverseScan */ + ) + require.NoError(t, err) + + updatedLeaseholder := tok.SyncTokenAndMaybeUpdateCache( + ctx, &roachpb.Lease{Replica: rep2, Sequence: 3}, &desc3, + ) + require.False(t, updatedLeaseholder) + require.NotNil(t, tok) + require.Equal(t, &desc3, tok.Desc()) + require.Nil(t, tok.Leaseholder()) + require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, tok.ClosedTimestampPolicy()) + }, + }, + { + name: "incompatible but fresher descriptor", + testFn: func(t *testing.T, cache *RangeCache) { + // Check that trying to update the cache with an incompatible but newer + // descriptor results in the token being invalidated. Additionally, we + // expect the cache entry corresponding to the older descriptor to be + // evicted and there to be a cache entry for the newer (incompatible) + // descriptor. + cache.Insert(ctx, roachpb.RangeInfo{ + Desc: desc2, + Lease: roachpb.Lease{ + Replica: rep3, + Sequence: 4, + }, + ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, + }) + tok, err := cache.LookupWithEvictionToken( + ctx, startKey, EvictionToken{}, false, /* useReverseScan */ + ) + require.NoError(t, err) + + l := roachpb.Lease{ + Replica: rep1, + Sequence: 2, + } + incompatibleDescriptor.Generation = desc2.Generation + 1 + updatedLeaseholder := tok.SyncTokenAndMaybeUpdateCache(ctx, &l, &incompatibleDescriptor) + require.False(t, updatedLeaseholder) + require.False(t, tok.Valid()) + + entries := cache.GetCachedOverlapping( + ctx, roachpb.RSpan{Key: roachpb.RKeyMin, EndKey: roachpb.RKeyMax}, + ) + require.Equal(t, 1, len(entries)) + require.Equal(t, incompatibleDescriptor, entries[0].desc) + require.Equal(t, l, entries[0].lease) + }, + }, + { + name: "incompatible but stale descriptor", + testFn: func(t *testing.T, cache *RangeCache) { + // Check that trying to update the cache with an incompatible but older + // descriptor results in no update being performed. + l := roachpb.Lease{ + Replica: rep3, + Sequence: 2, + } + cache.Insert(ctx, roachpb.RangeInfo{ + Desc: desc2, + Lease: l, + ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, + }) + tok, err := cache.LookupWithEvictionToken( + ctx, startKey, EvictionToken{}, false, /* useReverseScan */ + ) + require.NoError(t, err) + + incompatibleDescriptor.Generation = desc2.Generation - 1 + updatedLeaseholder := tok.SyncTokenAndMaybeUpdateCache( + ctx, &roachpb.Lease{Replica: rep2, Sequence: 4}, &incompatibleDescriptor, + ) + require.False(t, updatedLeaseholder) + require.True(t, tok.Valid()) + + entries := cache.GetCachedOverlapping( + ctx, roachpb.RSpan{Key: roachpb.RKeyMin, EndKey: roachpb.RKeyMax}, + ) + require.Equal(t, 1, len(entries)) + require.Equal(t, desc2, entries[0].desc) + require.Equal(t, l, entries[0].lease) + }, + }, + { + name: "speculative lease coming from a replica with a non-stale view", + testFn: func(t *testing.T, cache *RangeCache) { + // Check that trying to update the cache with a speculative lease coming + // from a replica that has a non-stale view of the world is persisted. + cache.Insert(ctx, roachpb.RangeInfo{ + Desc: desc2, + Lease: roachpb.Lease{ + Replica: rep3, + Sequence: 2, + }, + ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, + }) + tok, err := cache.LookupWithEvictionToken( + ctx, startKey, EvictionToken{}, false, /* useReverseScan */ + ) + require.NoError(t, err) + + updatedLeaseholder := tok.SyncTokenAndMaybeUpdateCacheWithSpeculativeLease( + ctx, rep2, &desc2, + ) + require.True(t, updatedLeaseholder) + require.Equal(t, &desc2, tok.Desc()) + require.Equal(t, &rep2, tok.Leaseholder()) + require.Equal(t, roachpb.LeaseSequence(0), tok.Lease().Sequence) + }, + }, } - // Check that there's no eviction if the range desc generation in the error is - // stale. - ok = tok.UpdateLease(ctx, l, staleRangeGeneration) - require.False(t, ok) - require.True(t, tok.Valid()) - - // However, expect an eviction when the error's desc generation is non-stale. - ok = tok.UpdateLease(ctx, l, nonStaleRangeGeneration) - require.False(t, ok) - require.False(t, tok.Valid()) - ri = cache.GetCached(ctx, startKey, false /* inverted */) - require.Nil(t, ri) - - // Check that updating the lease while the cache has a newer descriptor - // updates the token to the newer descriptor. - - cache.Insert(ctx, roachpb.RangeInfo{ - Desc: desc1, - Lease: roachpb.Lease{}, - ClosedTimestampPolicy: roachpb.LEAD_FOR_GLOBAL_READS, - }) - tok, err = cache.LookupWithEvictionToken( - ctx, desc1.StartKey, EvictionToken{}, false /* useReverseScan */) - require.NoError(t, err) - - // Update the cache. - cache.Insert(ctx, roachpb.RangeInfo{ - Desc: desc2, - Lease: roachpb.Lease{}, - }) - ok = tok.UpdateLease(ctx, &roachpb.Lease{Replica: rep2, Sequence: 3}, 0 /* descGeneration */) - require.True(t, ok) - require.NotNil(t, tok) - require.Equal(t, &desc2, tok.Desc()) - require.Equal(t, &rep2, tok.Leaseholder()) - require.Equal(t, tok.lease.Replica, rep2) - require.Equal(t, roachpb.LEAD_FOR_GLOBAL_READS, tok.ClosedTimestampPolicy()) - // Update the cache again. - cache.Insert(ctx, roachpb.RangeInfo{ - Desc: desc3, - Lease: roachpb.Lease{}, - }) - // This time try to specify a lease that's not compatible with the desc. The - // entry should end up evicted from the cache. - ok = tok.UpdateLease(ctx, &roachpb.Lease{Replica: rep3, Sequence: 4}, 0 /* descGeneration */) - require.False(t, ok) - require.False(t, tok.Valid()) - ri = cache.GetCached(ctx, startKey, false /* inverted */) - require.Nil(t, ri) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cache.Clear() + tc.testFn(t, cache) + }) + } } -func TestRangeCacheEntryUpdateLease(t *testing.T) { +func TestRangeCacheEntryMaybeUpdate(t *testing.T) { defer leaktest.AfterTest(t)() + ctx := context.Background() + rep1 := roachpb.ReplicaDescriptor{ NodeID: 1, StoreID: 1, @@ -1715,18 +1948,47 @@ func TestRangeCacheEntryUpdateLease(t *testing.T) { StoreID: 2, ReplicaID: 2, } - repNonMember := roachpb.ReplicaDescriptor{ + rep3 := roachpb.ReplicaDescriptor{ NodeID: 3, StoreID: 3, ReplicaID: 3, } + repStaleMember := roachpb.ReplicaDescriptor{ + NodeID: 4, + StoreID: 4, + ReplicaID: 4, + } + staleDesc := roachpb.RangeDescriptor{ + StartKey: roachpb.RKeyMin, + EndKey: roachpb.RKeyMax, + InternalReplicas: []roachpb.ReplicaDescriptor{ + rep1, repStaleMember, + }, + Generation: 2, + } desc := roachpb.RangeDescriptor{ StartKey: roachpb.RKeyMin, EndKey: roachpb.RKeyMax, InternalReplicas: []roachpb.ReplicaDescriptor{ rep1, rep2, }, - Generation: 0, + Generation: 3, + } + desc2 := roachpb.RangeDescriptor{ + StartKey: roachpb.RKeyMin, + EndKey: roachpb.RKeyMax, + InternalReplicas: []roachpb.ReplicaDescriptor{ + rep2, rep3, + }, + Generation: 4, + } + desc3 := roachpb.RangeDescriptor{ + StartKey: roachpb.RKeyMin, + EndKey: roachpb.RKeyMax, + InternalReplicas: []roachpb.ReplicaDescriptor{ + rep1, rep3, + }, + Generation: 5, } e := &CacheEntry{ @@ -1739,69 +2001,111 @@ func TestRangeCacheEntryUpdateLease(t *testing.T) { Replica: rep1, Sequence: 1, } - ok, e := e.updateLease(l, 0 /* descGeneration */) - require.True(t, ok) + updated, updatedLease, e := e.maybeUpdate(ctx, l, &desc) + require.True(t, updated) + require.True(t, updatedLease) require.True(t, l.Equal(e.Lease())) + require.True(t, desc.Equal(e.Desc())) - // Check that a lease with no sequence number overwrites any other lease. + // Check that another lease with no seq num overwrites any other lease when + // the associated range descriptor isn't stale. l = &roachpb.Lease{ - Replica: rep1, + Replica: rep2, Sequence: 0, } - ok, e = e.updateLease(l, 0 /* descGeneration */) - require.True(t, ok) + updated, updatedLease, e = e.maybeUpdate(ctx, l, &desc) + require.True(t, updated) + require.True(t, updatedLease) require.NotNil(t, e.Leaseholder()) require.True(t, l.Replica.Equal(*e.Leaseholder())) + require.True(t, desc.Equal(e.Desc())) // Check that Seq=0 leases are not returned by Lease(). require.Nil(t, e.Lease()) - // Check that another lease with no seq num overwrites a lease with no seq num. + // Check that another lease with no sequence number overwrites a lease with no + // sequence num as long as the associated range descriptor isn't stale. l = &roachpb.Lease{ - Replica: rep2, + Replica: rep1, Sequence: 0, } - ok, e = e.updateLease(l, 0 /* descGeneration */) - require.True(t, ok) + updated, updatedLease, e = e.maybeUpdate(ctx, l, &desc) + require.True(t, updated) + require.True(t, updatedLease) require.NotNil(t, e.Leaseholder()) require.True(t, l.Replica.Equal(*e.Leaseholder())) + require.True(t, desc.Equal(e.Desc())) + // Check that Seq=0 leases are not returned by Lease(). + require.Nil(t, e.Lease()) - // Check that another lease with no seq num overwrites a lease with no seq num. + oldL := l l = &roachpb.Lease{ - Replica: rep1, + Replica: repStaleMember, Sequence: 0, } - ok, e = e.updateLease(l, 0 /* descGeneration */) - require.True(t, ok) + // Ensure that a speculative lease is not overwritten when accompanied by a + // stale range descriptor. + updated, updatedLease, e = e.maybeUpdate(ctx, l, &staleDesc) + require.False(t, updated) + require.False(t, updatedLease) require.NotNil(t, e.Leaseholder()) - require.True(t, l.Replica.Equal(*e.Leaseholder())) + require.True(t, oldL.Replica.Equal(*e.Leaseholder())) + require.True(t, desc.Equal(e.Desc())) + // The old lease is still speculative; ensure it isn't returned by Lease(). + require.Nil(t, e.Lease()) - // Set a lease + // Ensure a speculative lease is not overwritten by a "real" lease if the + // accompanying range descriptor is stale. l = &roachpb.Lease{ Replica: rep1, + Sequence: 1, + } + updated, updatedLease, e = e.maybeUpdate(ctx, l, &staleDesc) + require.False(t, updated) + require.False(t, updatedLease) + require.NotNil(t, e.Leaseholder()) + require.True(t, oldL.Replica.Equal(*e.Leaseholder())) + require.True(t, desc.Equal(e.Desc())) + + // Empty out the lease and ensure that it is overwritten by a lease even if + // the accompanying range descriptor is stale. + e.lease = roachpb.Lease{} + updated, updatedLease, e = e.maybeUpdate(ctx, l, &staleDesc) + require.True(t, updated) + require.True(t, updatedLease) + require.NotNil(t, e.Leaseholder()) + require.True(t, oldL.Replica.Equal(*e.Leaseholder())) + require.True(t, e.Lease().Equal(l)) + // The range descriptor shouldn't be updated because the one supplied was + // stale. + require.True(t, desc.Equal(e.Desc())) + + // Ensure that a newer lease overwrites an older lease. + l = &roachpb.Lease{ + Replica: rep2, Sequence: 2, } - ok, e = e.updateLease(l, 0 /* descGeneration */) - require.True(t, ok) + updated, updatedLease, e = e.maybeUpdate(ctx, l, &desc) + require.True(t, updated) + require.True(t, updatedLease) require.NotNil(t, e.Leaseholder()) require.True(t, l.Equal(*e.Lease())) + require.True(t, desc.Equal(e.Desc())) // Check that updating to an older lease doesn't work. l = &roachpb.Lease{ - Replica: rep2, + Replica: rep1, Sequence: 1, } - ok, e = e.updateLease(l, 0 /* descGeneration */) - require.False(t, ok) + updated, updatedLease, e = e.maybeUpdate(ctx, l, &desc) + require.False(t, updated) + require.False(t, updatedLease) require.False(t, l.Equal(*e.Lease())) - // Check that updating to a lease at the same sequence as the existing one works. - l = &roachpb.Lease{ - Replica: rep2, - Sequence: 2, - } - ok, e = e.updateLease(l, 0 /* descGeneration */) - require.True(t, ok) - require.True(t, l.Equal(e.Lease())) + // Check that updating to an older descriptor doesn't work. + updated, updatedLease, e = e.maybeUpdate(ctx, l, &staleDesc) + require.False(t, updated) + require.False(t, updatedLease) + require.True(t, desc.Equal(e.Desc())) // Check that updating to the same lease returns false. l = &roachpb.Lease{ @@ -1809,19 +2113,33 @@ func TestRangeCacheEntryUpdateLease(t *testing.T) { Sequence: 2, } require.True(t, l.Equal(e.Lease())) - ok, e = e.updateLease(l, 0 /* descGeneration */) - require.False(t, ok) + updated, updatedLease, e = e.maybeUpdate(ctx, l, &desc) + require.False(t, updated) + require.False(t, updatedLease) + require.True(t, l.Equal(e.Lease())) + require.True(t, desc.Equal(e.Desc())) + + // Check that updating just the descriptor to a newer descriptor returns the + // correct values for updated and updatedLease. + updated, updatedLease, e = e.maybeUpdate(ctx, l, &desc2) + require.True(t, updated) + require.False(t, updatedLease) require.True(t, l.Equal(e.Lease())) + require.True(t, desc2.Equal(e.Desc())) - // Check that updating the lease to a non-member replica returns a nil - // entry. + // Check that updating the cache entry to a newer descriptor such that it + // makes the (freshest) lease incompatible clears out the lease on the + // returned cache entry. l = &roachpb.Lease{ - Replica: repNonMember, - Sequence: 0, + Replica: rep1, + Sequence: 1, } - ok, e = e.updateLease(l, 0 /* descGeneration */) - require.True(t, ok) - require.Nil(t, e) + require.Equal(t, roachpb.LeaseSequence(2), e.Lease().Sequence) + updated, updatedLease, e = e.maybeUpdate(ctx, l, &desc3) + require.True(t, updated) + require.False(t, updatedLease) + require.Nil(t, e.Lease()) + require.True(t, desc3.Equal(e.Desc())) } func TestRangeCacheEntryOverrides(t *testing.T) {