Skip to content

Commit

Permalink
kvccl: re-order enterprise check in canSendToFollower
Browse files Browse the repository at this point in the history
Fixes #62447.

In #62447, Erik found that #59571 had re-ordered the call to
`utilccl.CheckEnterpriseEnabled` to occur before checking the batch in
`canSendToFollower`, instead of after. This added an error allocation
into the hot path of all reads, which showed up in CPU profiles and
caused an 8% performance regression on `kv95`. This commit fixes this by
moving the enterprise check back out of the hot-path for all non-stale
read-only batches.

A follow up to this PR would be to make `utilccl.CheckEnterpriseEnabled`
cheaper by avoiding the error allocation for callers that don't need an
error. This work is not done in this commit.
  • Loading branch information
nvanbenschoten committed Mar 23, 2021
1 parent ee9f47b commit b7d2f2e
Showing 1 changed file with 8 additions and 6 deletions.
14 changes: 8 additions & 6 deletions pkg/ccl/kvccl/kvfollowerreadsccl/followerreads.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,10 @@ func canSendToFollower(
ctPolicy roachpb.RangeClosedTimestampPolicy,
ba roachpb.BatchRequest,
) bool {
return checkFollowerReadsEnabled(clusterID, st) &&
kvserver.BatchCanBeEvaluatedOnFollower(ba) &&
closedTimestampLikelySufficient(st, clock, ctPolicy, ba.Txn.RequiredFrontier())
return kvserver.BatchCanBeEvaluatedOnFollower(ba) &&
closedTimestampLikelySufficient(st, clock, ctPolicy, ba.Txn.RequiredFrontier()) &&
// NOTE: this call can be expensive, so perform it last. See #62447.
checkFollowerReadsEnabled(clusterID, st)
}

type followerReadOracle struct {
Expand Down Expand Up @@ -181,9 +182,10 @@ func (o *followerReadOracle) useClosestOracle(
// sent to the correct replicas once canSendToFollower is checked for each
// BatchRequests in the DistSender. This would hurt performance, but would
// not violate correctness.
return checkFollowerReadsEnabled(o.clusterID.Get(), o.st) &&
txn != nil &&
closedTimestampLikelySufficient(o.st, o.clock, ctPolicy, txn.RequiredFrontier())
return txn != nil &&
closedTimestampLikelySufficient(o.st, o.clock, ctPolicy, txn.RequiredFrontier()) &&
// NOTE: this call can be expensive, so perform it last. See #62447.
checkFollowerReadsEnabled(o.clusterID.Get(), o.st)
}

// followerReadOraclePolicy is a leaseholder choosing policy that detects
Expand Down

0 comments on commit b7d2f2e

Please sign in to comment.