kvserver: simplify requestCanProceed and leaseGoodToGo by tbg · Pull Request #72978 · cockroachdb/cockroach

tbg · 2021-11-19T15:32:36Z

I found myself having to make a change to this method in #72972 and was
really unsure about when this returned a nonzero lease status and
whether that even mattered. Also, the code was hard to grok due to
the large number of "fallthrough" if-else statements.

Now it follows a standard pattern of checking and immediately returning
errors as early as possible, and prefers early returns for the success
cases as well.

The method now clearly states what its contract (now) is: it will either
return a valid lease status and no error (leaseholder read), or a zero
lease status and no error (follower read or lease check skipped), or a
zero lease status and an error (can't serve request, for example invalid
lease, out of bounds, or pending merge, etc). Leases previously returned
a half-populated lease status up for the purpose of communicating the
sequence to use during proposal. However, it was cleaner to return a
zero status and to repurpose the existing special casing for leases
in evalAndPropose path to pull the sequence from the lease request.

I checked the callers to make sure that none of the now absent lease
statuses introduce a behavior change. The TL;DR is that callers always
ignored all but the valid lease status.

The simplest caller is executeAdminBatch which doesn't even assign
the returned lease status:

cockroach/pkg/kv/kvserver/replica_send.go

Lines 759 to 775 in 55720d0

    
           	_, err := r.checkExecutionCanProceed(ctx, ba, nil /* g */) 
        
           	if err == nil { 
        
           		break 
        
           	} 
        
           	switch { 
        
           	case errors.HasType(err, (*roachpb.InvalidLeaseError)(nil)): 
        
           		// If the replica does not have the lease, attempt to acquire it, or 
        
           		// redirect to the current leaseholder by returning an error. 
        
           		_, pErr := r.redirectOnOrAcquireLeaseForRequest(ctx, ba.Timestamp) 
        
           		if pErr != nil { 
        
           			return nil, pErr 
        
           		} 
        
           		// Retry... 
        
           	default: 
        
           		return nil, roachpb.NewError(err) 
        
           	} 
        
           }

The second caller is executeReadOnlyBatch:

cockroach/pkg/kv/kvserver/replica_read.go

Lines 42 to 46 in 86e2ede

    
           // Verify that the batch can be executed. 
        
           st, err := r.checkExecutionCanProceed(ctx, ba, g) 
        
           if err != nil { 
        
           	return nil, g, roachpb.NewError(err) 
        
           }

st is used only if err == nil, in two places:

cockroach/pkg/kv/kvserver/observedts/limit.go

Lines 42 to 50 in 64aadfe

    
           func ComputeLocalUncertaintyLimit( 
        
           	txn *roachpb.Transaction, status kvserverpb.LeaseStatus, 
        
           ) hlc.Timestamp { 
        
           	if txn == nil { 
        
           		return hlc.Timestamp{} 
        
           	} 
        
           	if status.State != kvserverpb.LeaseState_VALID { 
        
           		return hlc.Timestamp{} 
        
           	}

This will return early in case of a zero lease status, i.e. is a no-op
in this case.

The second use is storing st into the endCmds:

cockroach/pkg/kv/kvserver/replica_read.go

Line 152 in 86e2ede

ec, g := endCmds{repl: r, g: g, st: st}, nil

which is accessed only under a similar lease validity check:

cockroach/pkg/kv/kvserver/replica_send.go

Lines 1019 to 1021 in 55720d0

    
           if ba.ReadConsistency == roachpb.CONSISTENT && ec.st.State == kvserverpb.LeaseState_VALID { 
        
           	ec.repl.updateTimestampCache(ctx, &ec.st, ba, br, pErr) 
        
           }

The third caller is executeWriteBatch:

cockroach/pkg/kv/kvserver/replica_write.go

Lines 81 to 89 in 455cddd

    
           st, err := r.checkExecutionCanProceed(ctx, ba, g) 
        
           if err != nil { 
        
           	r.readOnlyCmdMu.RUnlock() 
        
           	return nil, g, roachpb.NewError(err) 
        
           } 
        
           // Compute the transaction's local uncertainty limit using observed 
        
           // timestamps, which can help avoid uncertainty restarts. 
        
           localUncertaintyLimit := observedts.ComputeLocalUncertaintyLimit(ba.Txn, st)

We handled ComputeLocalUncertaintyLimit already. Other than that, st
is passed to evalAndPropose. In that method, since it is the write
path, we shouldn't see follower reads and so the remaining case are
leases, where the status will be zero and the special-casing in that
method takes over.

This commit also simplifies leaseGoodToGoRLocked. It was repeatedly
reassigning status which I found hard to follow.

Release note: None

cockroach-teamcity · 2021-11-19T15:32:44Z

This change is

nvb

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

pkg/kv/kvserver/replica.go, line 1349 at r1 (raw file):

	// and we only hold a read-only lock.
	if shouldExtend {
		_ = r.store.stopper.RunAsyncTask(ctx, "maybe-extend-async", func(ctx context.Context) {

How do you feel about this double-async-task-to-promote-mutex approach? It kind of feels like an anti-pattern to me, but I also see why you did it given the structure of this code. Are we concerned about the potential for an unbounded number of async tasks being launched all at once when an expiration-based lease needs to be extended?

pkg/kv/kvserver/replica.go, line 1374 at r1 (raw file):

	st := r.leaseStatusForRequestRLocked(ctx, now, reqTS)

	// Check if request is below the GC threshold and if so, error out.

It's surprising to me that we're now checking the GC threshold before checking the lease status. With "strict GC enforcement" enabled, the GC threshold is a function of the lease status, so it feels like we should be checking the GC threshold after the lease. Was there a reason to make this change?

Was it so we could short-circuit on the next two checks?

I found myself having to make a change to this method in cockroachdb#72972 and was really unsure about when this returned a nonzero lease status and whether that even mattered. Also, the code was hard to grok due to the large number of "fallthrough" if-else statements. Now it follows a standard pattern of checking and immediately returning errors as early as possible, and prefers early returns for the success cases as well. The method now clearly states what its contract (now) is: it will either return a valid lease status and no error (leaseholder read), or a zero lease status and no error (follower read or lease check skipped), or a zero lease status and an error (can't serve request, for example invalid lease, out of bounds, or pending merge, etc). Leases previously returned a half-populated lease status up for the purpose of communicating the sequence to use during proposal. However, it was cleaner to return a zero status and to repurpose the existing special casing for leases in `evalAndPropose` path to pull the sequence from the lease request. I checked the callers to make sure that none of the now absent lease statuses introduce a behavior change. The TL;DR is that callers always ignored all but the valid lease status. ---- The simplest caller is `executeAdminBatch` which doesn't even assign the returned lease status: https://github.com/cockroachdb/cockroach/blob/55720d0a4c0dd8030cd19645461226d173eefc2e/pkg/kv/kvserver/replica_send.go#L759-L775 The second caller is `executeReadOnlyBatch`: https://github.com/cockroachdb/cockroach/blob/86e2edece61091944d8575336b5de74fafc28b35/pkg/kv/kvserver/replica_read.go#L42-L46 `st` is used only if `err == nil`, in two places: https://github.com/cockroachdb/cockroach/blob/64aadfec4b3877371b6f71bf5f9aa83bd97aa515/pkg/kv/kvserver/observedts/limit.go#L42-L50 This will return early in case of a zero lease status, i.e. is a no-op in this case. The second use is storing `st` into the `endCmds`: https://github.com/cockroachdb/cockroach/blob/86e2edece61091944d8575336b5de74fafc28b35/pkg/kv/kvserver/replica_read.go#L152 which is accessed only under a similar lease validity check: https://github.com/cockroachdb/cockroach/blob/55720d0a4c0dd8030cd19645461226d173eefc2e/pkg/kv/kvserver/replica_send.go#L1019-L1021 The third caller is `executeWriteBatch`: https://github.com/cockroachdb/cockroach/blob/455cdddc6d75c03645f486b22970e5c6198a8d56/pkg/kv/kvserver/replica_write.go#L81-L89 We handled `ComputeLocalUncertaintyLimit` already. Other than that, `st` is passed to `evalAndPropose`. In that method, since it is the write path, we shouldn't see follower reads and so the remaining case are leases, where the status will be zero and the special-casing in that method takes over. ---- This commit also simplifies leaseGoodToGoRLocked. It was repeatedly reassigning `status` which I found hard to follow. Release note: None

tbg

TFTR, RFAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

pkg/kv/kvserver/replica.go, line 1349 at r1 (raw file):
Didn't love it, thanks for the nudge - I think this is better now.

Are we concerned about the potential for an unbounded number of async tasks being launched all at once when an expiration-based lease needs to be extended?

We aren't in either version of the code, as

cockroach/pkg/kv/kvserver/replica_range_lease.go

Lines 1282 to 1295 in a5e32e1

    
           // shouldExtendLeaseRLocked determines whether the lease should be 
        
           // extended asynchronously, even if it is currently valid. The method 
        
           // returns true if this range uses expiration-based leases, the lease is 
        
           // in need of renewal, and there's not already an extension pending. 
        
           func (r *Replica) shouldExtendLeaseRLocked(st kvserverpb.LeaseStatus) bool { 
        
           	if !r.requiresExpiringLeaseRLocked() { 
        
           		return false 
        
           	} 
        
           	if _, ok := r.mu.pendingLeaseRequest.RequestPending(); ok { 
        
           		return false 
        
           	} 
        
           	renewal := st.Lease.Expiration.Add(-r.store.cfg.RangeLeaseRenewalDuration().Nanoseconds(), 0) 
        
           	return renewal.LessEq(st.Now.ToTimestamp()) 
        
           }

is smart enough.

pkg/kv/kvserver/replica.go, line 1374 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It's surprising to me that we're now checking the GC threshold before checking the lease status. With "strict GC enforcement" enabled, the GC threshold is a function of the lease status, so it feels like we should be checking the GC threshold after the lease. Was there a reason to make this change?

Was it so we could short-circuit on the next two checks?

Yeah, with the check later we end up with a more complicated control flow again, which I was trying to avoid. I think there is also an argument that when any replica can't serve the request due to GCThreshold, that this should take precedence over the lease, as the leaseholder likely also can't serve it, so we're just wasting less time. A counter-argument could be that in principle there is no reason why the GC threshold has to be consistent across replicas, one could imagine specializing some replicas to serve historical reads and keeping everyone else's histories small.

However, I also don't like the early returns here much. The control flow I really want is one where all early returns are on error and success means reaching the end of the method. Gave this a do-over; a little bit of the fallthrough branching that I didn't like is back but I think it is manageable & I prefer it over the version you commented on here.

nvb

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

pkg/kv/kvserver/replica.go, line 1298 at r2 (raw file):

	postRUnlock := func() {}
	r.mu.RLock()
	defer func() {

Sorry to ask, but this doesn't incur a heap allocation, right?

tbg

TFTR!

bors r=nvanbenschoten

CI is red due to what is likely an unrelated failure, which I have also seen in another PR (and notified someone about).

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

pkg/kv/kvserver/replica.go, line 1298 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Sorry to ask, but this doesn't incur a heap allocation, right?

Good question, no it doesn't seem like it. I checked using goescape and also scrutinized the manual output of go build -gcflags='-m'. Interestingly postRUnlock does not occur in the output at all, I hope that just means it doesn't escape and doesn't mean I'm holding it wrong (plenty of other vars in replica.go are mentioned).

craig · 2021-11-24T16:56:00Z

Build succeeded:

GitHub CI (Cockroach)

PR cockroachdb#72978 unintentionally introduced a small change: on a follower read, we would pass the lease status to `checkTSAboveGCThreshold`. If this lease status was `VALID` but not owned by the recipient replica (i.e. this was a follower read), `checkTSAboveGCThreshold` could return the implied GCThreshold (i.e. `now()-gcttl`) instead of the lowest possible TTL. This made `TestStrictGCEnforcement` flaky since that test verifies that a historical read goes through on the recent recipient of a lease (which may not have applied the lease yet), who would serve it via a follower read and would thus error out a read that the test expected to go through. Fixes cockroachdb#73123. Release note: None

73153: kvserver: de-flake TestStrictGCEnforcement r=erikgrinaker a=tbg PR #72978 unintentionally introduced a small change: on a follower read, we would pass the lease status to `checkTSAboveGCThreshold`. If this lease status was `VALID` but not owned by the recipient replica (i.e. this was a follower read), `checkTSAboveGCThreshold` could return the implied GCThreshold (i.e. `now()-gcttl`) instead of the lowest possible TTL. This made `TestStrictGCEnforcement` flaky since that test verifies that a historical read goes through on the recent recipient of a lease (which may not have applied the lease yet), who would serve it via a follower read and would thus error out a read that the test expected to go through. Fixes #73123. Release note: None Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>

tbg force-pushed the exec-lease-status branch 3 times, most recently from e224ebe to 914530d Compare November 22, 2021 10:20

tbg marked this pull request as ready for review November 22, 2021 10:35

tbg requested a review from a team as a code owner November 22, 2021 10:35

tbg requested a review from nvb November 22, 2021 10:35

tbg force-pushed the exec-lease-status branch from 914530d to 00c9da5 Compare November 22, 2021 10:41

nvb reviewed Nov 22, 2021

View reviewed changes

tbg force-pushed the exec-lease-status branch from 00c9da5 to 7cbdb8c Compare November 23, 2021 09:09

tbg requested a review from nvb November 23, 2021 09:10

tbg commented Nov 23, 2021

View reviewed changes

nvb approved these changes Nov 24, 2021

View reviewed changes

tbg commented Nov 24, 2021

View reviewed changes

craig bot merged commit 0331536 into cockroachdb:master Nov 24, 2021

tbg mentioned this pull request Nov 25, 2021

kv/kvserver: TestStrictGCEnforcement failed #73123

Closed

tbg mentioned this pull request Nov 25, 2021

kvserver: de-flake TestStrictGCEnforcement #73153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: simplify requestCanProceed and leaseGoodToGo#72978

kvserver: simplify requestCanProceed and leaseGoodToGo#72978
craig[bot] merged 1 commit intocockroachdb:masterfrom
tbg:exec-lease-status

tbg commented Nov 19, 2021 •

edited

Loading

Uh oh!

cockroach-teamcity commented Nov 19, 2021

Uh oh!

nvb left a comment

Uh oh!

tbg left a comment

Uh oh!

nvb left a comment

Uh oh!

tbg left a comment

Uh oh!

craig bot commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	_, err := r.checkExecutionCanProceed(ctx, ba, nil /* g */)
	if err == nil {
	break
	}
	switch {
	case errors.HasType(err, (*roachpb.InvalidLeaseError)(nil)):
	// If the replica does not have the lease, attempt to acquire it, or
	// redirect to the current leaseholder by returning an error.
	_, pErr := r.redirectOnOrAcquireLeaseForRequest(ctx, ba.Timestamp)
	if pErr != nil {
	return nil, pErr
	}
	// Retry...
	default:
	return nil, roachpb.NewError(err)
	}
	}

	// Verify that the batch can be executed.
	st, err := r.checkExecutionCanProceed(ctx, ba, g)
	if err != nil {
	return nil, g, roachpb.NewError(err)
	}

	func ComputeLocalUncertaintyLimit(
	txn *roachpb.Transaction, status kvserverpb.LeaseStatus,
	) hlc.Timestamp {
	if txn == nil {
	return hlc.Timestamp{}
	}
	if status.State != kvserverpb.LeaseState_VALID {
	return hlc.Timestamp{}
	}

	if ba.ReadConsistency == roachpb.CONSISTENT && ec.st.State == kvserverpb.LeaseState_VALID {
	ec.repl.updateTimestampCache(ctx, &ec.st, ba, br, pErr)
	}

	st, err := r.checkExecutionCanProceed(ctx, ba, g)
	if err != nil {
	r.readOnlyCmdMu.RUnlock()
	return nil, g, roachpb.NewError(err)
	}

	// Compute the transaction's local uncertainty limit using observed
	// timestamps, which can help avoid uncertainty restarts.
	localUncertaintyLimit := observedts.ComputeLocalUncertaintyLimit(ba.Txn, st)

	// shouldExtendLeaseRLocked determines whether the lease should be
	// extended asynchronously, even if it is currently valid. The method
	// returns true if this range uses expiration-based leases, the lease is
	// in need of renewal, and there's not already an extension pending.
	func (r *Replica) shouldExtendLeaseRLocked(st kvserverpb.LeaseStatus) bool {
	if !r.requiresExpiringLeaseRLocked() {
	return false
	}
	if _, ok := r.mu.pendingLeaseRequest.RequestPending(); ok {
	return false
	}
	renewal := st.Lease.Expiration.Add(-r.store.cfg.RangeLeaseRenewalDuration().Nanoseconds(), 0)
	return renewal.LessEq(st.Now.ToTimestamp())
	}

Conversation

tbg commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Nov 19, 2021

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

tbg left a comment

Choose a reason for hiding this comment

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

tbg left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tbg commented Nov 19, 2021 •

edited

Loading