roachpb: cleanup speculative leases returned by NotLeaseHolderErrors

[arulajmani]

Speculative leases are returned as part of NLHEs if the committed lease
is not known, but there is a guess as to who the leaseholder may be.
Previously, there were two ways to return these -- either by populating
just the LeaseHolder field on the NLHE or by populating the Lease field
with an unset sequence number. This patch unifies this behavior, and
going forward, we expect the latter to be the canonical form to
represent speculative leases. To that effect, the LeaseHolder field in
the NLHE proto is prefixed as "deprecated". We should be able to remove
the thing entirely in v23.1.

Release note: None
Release justification: low risk change.

[cockroach-teamcity]

This change is 

[arulajmani]

Initially, I was leaning towards using the LeaseHolder field on the NLHE for speculative leases, but thinking about it a bit more what I have here feels cleaner. It allows us to stop interpreting what kind of lease we got in the NLHE in the DistSender and simply pass the thing down to the range cache as-is (once we no longer have to care about the mixed version case). Also, I realized we have a lease.Speculative() method already, so I think it's better to reuse that concept instead of adding another one on the NLHE proto.

[nvanbenschoten]

I went back on forth on this question too. You could make an argument that the LeaseHolder field provides stronger typing. However, the fact that lease.Speculative() exists is compelling. It means that "speculative leases" are a more general concept, and aren't just constrained to this error path. Nice cleanup.

Reviewed 11 of 11 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

---

pkg/kv/kvserver/replica_range_lease.go line 1004 at r1 (raw file):

// current lease is not known, but the error is being created by guessing who
// the leaseholder may be.
func newNotLeaseHolderErrorWithSpeculativeLease(

Two nits:

1. we should have this create the speculative lease, but then call newNotLeaseHolderError, instead of duplicating logic.
2. this should live below newNotLeaseHolderError, because that's the more commonly used function and this is a special-case variant.

---

pkg/kv/kvserver/replica_range_lease.go line 1026 at r1 (raw file):

			err.Lease = new(roachpb.Lease)
			*err.Lease = l
			err.LeaseHolder = &err.Lease.Replica

Should we continue to return this (with the new name) until next release? Do v22.1 clients still care about it.

[arulajmani]

Yeah, and it makes sense for these things to be a general concept given how they're stored/used by the RangeCache.

TFTR!

bors r=nvanbenschoten

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

---

pkg/kv/kvserver/replica_range_lease.go line 1004 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Two nits:

1. we should have this create the speculative lease, but then call newNotLeaseHolderError, instead of duplicating logic.
2. this should live below newNotLeaseHolderError, because that's the more commonly used function and this is a special-case variant.

Good calls, changed.

---

pkg/kv/kvserver/replica_range_lease.go line 1026 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we continue to return this (with the new name) until next release? Do v22.1 clients still care about it.

Clients don't care about this thing if err.Lease is set, so removing this should be fine.

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)