kvserver: serve follower reads before checking lease

[nvanbenschoten]

Extension of #57397.

Once closed timestamps are separated away from lease state, it becomes possible to serve follower reads on ranges that have no active leaseholder. To do so, we'd want to check whether a read can be served by a replica based on the replica's closed timestamp before checking whether the replica has or knows about the current Range lease.

An important property that this should give us is that once a replica begins serving follower reads at a timestamp, it will always continue to serve follower reads at that timestamp. This means that even if the replica becomes completely partitioned away from the rest of its range, it will continue to be available for (increasingly) stale reads.

Epic: CRDB-2559

[andreimatei]

I tried switching around the conditions for serving as a leaseholder and serving as a follower below, and hit a snag:

cockroach/pkg/kv/kvserver/replica.go

Lines 1312 to 1317 in 679ee6e

	st, shouldExtend, err = r.leaseGoodToGoRLocked(ctx, now, ba.WriteTimestamp())
	if err != nil {
	// If not, can we serve this request on a follower?
	// TODO(nvanbenschoten): once we make this check cheaper
	// than leaseGoodToGoRLocked, invert these checks.
	if !r.canServeFollowerReadRLocked(ctx, ba, err) {

As things stand, if I just invert the conditions and have the leaseholder evaluate a request under an empty lease status, that request just spins in case it runs into a lock. This is because, when hitting the lock and trying to handle it via handleWriteIntentError(), this obscure code path is hit:

cockroach/pkg/kv/kvserver/concurrency/lock_table.go

Line 2335 in 368e589

return true, nil

To the lock table, the situation looks as if a request evaluating under an old lease hit an intent and the expectation of the highlighted code is that the request will be re-evaluated under a newer lease and thus take a different code path (I think). However, without further changes, the request continues to evaluate as a follower read again and again.

I'm wondering what the moral is here. It seems to me that there are benefits from evaluating under a lease when there is a lease: the encountered locks can be added to the lock table. I guess the alternative is to arrange things such that a request evaluating on the leaseholder as a "follower read" handles locks the same way that a true follower would - I believe on follower the lock table is disabled and thus not involved in waiting for locks, and so the reader pushes directly. But, I assume there are benefits to going through the lock table (which I'd be curious to hear spelled out). I see notes, though, suggesting that maybe we actually want to go more in the other direction - enabling the lock table on followers.

cockroach/pkg/kv/kvserver/concurrency/concurrency_control.go

Lines 765 to 773 in 9157c7b

	// NOTE: this method is used when the lockTable is disabled (e.g. on a
	// follower replica) and a lock is discovered that must be waited on (e.g.
	// during a follower read). If/when lockTables are maintained on follower
	// replicas by propagating lockTable state transitions through the Raft log
	// in the ReplicatedEvalResult instead of through the (leaseholder-only)
	// LocalResult, we should be able to remove the lockTable "disabled" state
	// and, in turn, remove this method. This will likely fall out of pulling
	// all replicated locks into the lockTable.
	WaitOnLock(context.Context, Request, *roachpb.Intent) *Error

The note says that this might happen with the migration of replicated locks in to the lock table, so maybe it's close?

I want take the opportunity to discuss this issue more conceptually, to have it click more for me. The issue is motivated as:

An important property that this should give us is that once a replica begins serving follower reads at a timestamp, it will always continue to serve follower reads at that timestamp. This means that even if the replica becomes completely partitioned away from the rest of its range, it will continue to stale available for (increasingly) stale reads.

I believe what this passage refers to is the fact that, currently, a replica doesn't activate the follower reads code path when the lease it knows about is invalid. This is because of the following code:

cockroach/pkg/kv/kvserver/replica_follower_read.go

Lines 68 to 70 in e024445

	var lErr *roachpb.NotLeaseHolderError
	eligible := errors.As(err, &lErr) &&
	lErr.LeaseHolder != nil && lErr.Lease != nil && lErr.Lease.Type() == roachpb.LeaseEpoch &&

This code doesn't handle the InvalidLeaseError (produced here) and, even for the NotLeaseholderError, it nitpicks the error's fields. This code is crap, as far as I can tell. The only reason the code has to care about the specifics of the lease error has to do with the following line:

r.store.cfg.ClosedTimestamp.Clients.Request(lErr.LeaseHolder.NodeID, r.RangeID)

This is about the "old closed timestamp code", and is going away momentarily. So, I can simply remove the error checks inside canServeFollowerReads() and thus get the property that the current status of the lease (or rather the replica's view of the lease status) will have no bearing on serving follower reads. Which is what we want, right? The reordering of the checks that this issue is ostensibly asking for doesn't really seem necessary. Does this sound right?

[nvanbenschoten]

That all sounds right to me. Once we stop considering the error type in canServeFollowerReads, it doesn't really matter whether the lease is checked before or after the closed timestamp check. From what you wrote above, it sounds like it is valuable to know whether we are evaluating on a leaseholder even if we are evaluating under the closed timestamp, so I don't see a reason to re-order the checks.