From 09e60c0616ad2b6d76ce3abae5b7336f0181ebe1 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Thu, 9 Jun 2022 23:49:00 -0400 Subject: [PATCH] kv: clean up BatchRequest.IsLeaseRequest This commit cleans up `BatchRequest.IsLeaseRequest` by replacing its internals with a call to `isSingleRequestWithMethod(RequestLease)` and renaming it to mirror the rest of the `BatchRequest.IsSingleXYZRequest` methods. --- .../replica_application_state_machine.go | 2 +- pkg/kv/kvserver/replica_proposal.go | 2 +- pkg/kv/kvserver/replica_proposal_buf.go | 6 +++--- pkg/kv/kvserver/replica_rangefeed_test.go | 4 ++-- pkg/roachpb/batch.go | 21 ++++++++----------- 5 files changed, 16 insertions(+), 19 deletions(-) diff --git a/pkg/kv/kvserver/replica_application_state_machine.go b/pkg/kv/kvserver/replica_application_state_machine.go index f9fbd9f81497..f98ce4b7f89d 100644 --- a/pkg/kv/kvserver/replica_application_state_machine.go +++ b/pkg/kv/kvserver/replica_application_state_machine.go @@ -1478,7 +1478,7 @@ func (s *closedTimestampSetterInfo) record(cmd *replicatedCmd, lease *roachpb.Le s.merge = true } } - } else if req.IsLeaseRequest() { + } else if req.IsSingleRequestLeaseRequest() { // Make a deep copy since we're not allowed to hold on to request // memory. lr, _ := req.GetArg(roachpb.RequestLease) diff --git a/pkg/kv/kvserver/replica_proposal.go b/pkg/kv/kvserver/replica_proposal.go index b52ec9fd2d2c..ba050e228b0b 100644 --- a/pkg/kv/kvserver/replica_proposal.go +++ b/pkg/kv/kvserver/replica_proposal.go @@ -715,7 +715,7 @@ func (r *Replica) evaluateProposal( // Set the proposal's replicated result, which contains metadata and // side-effects that are to be replicated to all replicas. - res.Replicated.IsLeaseRequest = ba.IsLeaseRequest() + res.Replicated.IsLeaseRequest = ba.IsSingleRequestLeaseRequest() if ba.AppliesTimestampCache() { res.Replicated.WriteTimestamp = ba.WriteTimestamp() } else { diff --git a/pkg/kv/kvserver/replica_proposal_buf.go b/pkg/kv/kvserver/replica_proposal_buf.go index b515ed750ba0..e9897343ef17 100644 --- a/pkg/kv/kvserver/replica_proposal_buf.go +++ b/pkg/kv/kvserver/replica_proposal_buf.go @@ -450,7 +450,7 @@ func (b *propBuf) FlushLockedWithRaftGroup( // Lease extensions for a currently held lease always go through, to // keep the lease alive until the normal lease transfer mechanism can // colocate it with the leader. - if !leaderInfo.iAmTheLeader && p.Request.IsLeaseRequest() { + if !leaderInfo.iAmTheLeader && p.Request.IsSingleRequestLeaseRequest() { leaderKnownAndEligible := leaderInfo.leaderKnown && leaderInfo.leaderEligibleForLease ownsCurrentLease := b.p.ownsValidLeaseRLocked(ctx, b.clock.NowAsClockTimestamp()) if leaderKnownAndEligible && !ownsCurrentLease && !b.testing.allowLeaseProposalWhenNotLeader { @@ -618,7 +618,7 @@ func (b *propBuf) allocateLAIAndClosedTimestampLocked( // replays (though they could in principle also be handled like lease // requests). var lai uint64 - if !p.Request.IsLeaseRequest() { + if !p.Request.IsSingleRequestLeaseRequest() { b.assignedLAI++ lai = b.assignedLAI } @@ -673,7 +673,7 @@ func (b *propBuf) allocateLAIAndClosedTimestampLocked( // timestamps carried by lease requests, make sure to resurrect the old // TestRejectedLeaseDoesntDictateClosedTimestamp and protect against that // scenario. - if p.Request.IsLeaseRequest() { + if p.Request.IsSingleRequestLeaseRequest() { return lai, hlc.Timestamp{}, nil } diff --git a/pkg/kv/kvserver/replica_rangefeed_test.go b/pkg/kv/kvserver/replica_rangefeed_test.go index b490a433a015..06d1f0fa3ea1 100644 --- a/pkg/kv/kvserver/replica_rangefeed_test.go +++ b/pkg/kv/kvserver/replica_rangefeed_test.go @@ -1208,7 +1208,7 @@ func TestRangefeedCheckpointsRecoverFromLeaseExpiration(t *testing.T) { return nil } nudged := atomic.LoadInt64(&nudgeSeen) - if ba.IsLeaseRequest() && (nudged == 1) { + if ba.IsSingleRequestLeaseRequest() && (nudged == 1) { return nil } log.Infof(ctx, "test rejecting request: %s", ba) @@ -1374,7 +1374,7 @@ func TestNewRangefeedForceLeaseRetry(t *testing.T) { return nil } nudged := atomic.LoadInt64(&nudgeSeen) - if ba.IsLeaseRequest() && (nudged == 1) { + if ba.IsSingleRequestLeaseRequest() && (nudged == 1) { return nil } log.Infof(ctx, "test rejecting request: %s", ba) diff --git a/pkg/roachpb/batch.go b/pkg/roachpb/batch.go index 5cb3bdc72f84..5044586ad74b 100644 --- a/pkg/roachpb/batch.go +++ b/pkg/roachpb/batch.go @@ -147,18 +147,6 @@ func (ba *BatchRequest) UpdateTxn(o *Transaction) { ba.Txn = clonedTxn } -// IsLeaseRequest returns whether the batch consists of a single RequestLease -// request. Note that TransferLease requests return false. -// RequestLease requests are special because they're the only type of requests a -// non-lease-holder can propose. -func (ba *BatchRequest) IsLeaseRequest() bool { - if !ba.IsSingleRequest() { - return false - } - _, ok := ba.GetArg(RequestLease) - return ok -} - // AppliesTimestampCache returns whether the command is a write that applies the // timestamp cache (and closed timestamp), possibly pushing its write timestamp // into the future to avoid re-writing history. @@ -228,6 +216,15 @@ func (ba *BatchRequest) isSingleRequestWithMethod(m Method) bool { return ba.IsSingleRequest() && ba.Requests[0].GetInner().Method() == m } +// IsSingleRequestLeaseRequest returns true iff the batch contains a single +// request, and that request is a RequestLease. Note that TransferLease requests +// return false. +// RequestLease requests are special because they're the only type of requests a +// non-lease-holder can propose. +func (ba *BatchRequest) IsSingleRequestLeaseRequest() bool { + return ba.isSingleRequestWithMethod(RequestLease) +} + // IsSingleTransferLeaseRequest returns true iff the batch contains a single // request, and that request is a TransferLease. func (ba *BatchRequest) IsSingleTransferLeaseRequest() bool {