Skip to content

Commit

Permalink
kvclient: ship range descriptor in NLHE
Browse files Browse the repository at this point in the history
We recently changed a NLHE to carry the range descriptor generation to
ensure to avoid thrashing the range cache if the replica had a stale
view of the range. In cockroachdb#75742, we saw issues caused by the dist sender
having a stale range descriptor. This patch switches from sending just
the descriptor generation back in NLHE to shipping back the entire
range descriptor. In the future, we may want to solve the issue above
by updating the range cache with the fresher range descriptor thus
skipping a cache eviction and range descriptor lookup.

References cockroachdb#75742

Release note: None
  • Loading branch information
arulajmani authored and aayushshah15 committed Sep 28, 2022
1 parent 002021b commit 95c9392
Show file tree
Hide file tree
Showing 5 changed files with 285 additions and 224 deletions.
4 changes: 2 additions & 2 deletions pkg/kv/kvclient/kvcoord/dist_sender.go
Original file line number Diff line number Diff line change
Expand Up @@ -2145,10 +2145,10 @@ func (ds *DistSender) sendToReplicas(

var updatedLeaseholder bool
if tErr.Lease != nil {
updatedLeaseholder = routing.UpdateLease(ctx, tErr.Lease, tErr.DescriptorGeneration)
updatedLeaseholder = routing.UpdateLease(ctx, tErr.Lease, tErr.RangeDesc.Generation)
} else if tErr.LeaseHolder != nil {
// tErr.LeaseHolder might be set when tErr.Lease isn't.
routing.UpdateLeaseholder(ctx, *tErr.LeaseHolder, tErr.DescriptorGeneration)
routing.UpdateLeaseholder(ctx, *tErr.LeaseHolder, tErr.RangeDesc.Generation)
updatedLeaseholder = true
}
// Move the new leaseholder to the head of the queue for the next
Expand Down
7 changes: 6 additions & 1 deletion pkg/kv/kvclient/kvcoord/dist_sender_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1088,7 +1088,12 @@ func TestDistSenderIgnoresNLHEBasedOnOldRangeGeneration(t *testing.T) {
calls = append(calls, ba.Replica.NodeID)
if ba.Replica.NodeID == 2 {
reply := &roachpb.BatchResponse{}
err := &roachpb.NotLeaseHolderError{Lease: &ambiguousLease, DescriptorGeneration: oldGeneration}
err := &roachpb.NotLeaseHolderError{
Lease: &ambiguousLease,
RangeDesc: roachpb.RangeDescriptor{
Generation: oldGeneration,
},
}
reply.Error = roachpb.NewError(err)
return reply, nil
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/kv/kvserver/replica_range_lease.go
Original file line number Diff line number Diff line change
Expand Up @@ -914,9 +914,9 @@ func newNotLeaseHolderError(
l roachpb.Lease, proposerStoreID roachpb.StoreID, rangeDesc *roachpb.RangeDescriptor, msg string,
) *roachpb.NotLeaseHolderError {
err := &roachpb.NotLeaseHolderError{
RangeID: rangeDesc.RangeID,
DescriptorGeneration: rangeDesc.Generation,
CustomMsg: msg,
RangeID: rangeDesc.RangeID,
RangeDesc: *rangeDesc,
CustomMsg: msg,
}
if proposerStoreID != 0 {
err.Replica, _ = rangeDesc.GetReplicaDescriptor(proposerStoreID)
Expand Down
Loading

0 comments on commit 95c9392

Please sign in to comment.