kvserver: return destination from AdminScatter

[pbardea]

This commit updates the AdminScatter request to return the node of the
leaseholder after the scattering for each range that is scattered in the
request.

Release note: None

[cockroach-teamcity]

This change is 

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @dt, and @pbardea)

---

pkg/roachpb/api.proto, line 1549 at r1 (raw file):

    reserved 2;
    // DestinationNodeID is the range's leaseholder after the scattering.
    int32 destination_node_id = 3 [

call this leaseholder.

But let's do this for real. Let's deprecate this message Range, and return a repeated RangeInfo - which is a descriptor and a lease. r.adminScatter can use r.GetDescAndLease() to get a consistent view of both a descriptor and a lease.

[pbardea]

Thank you for taking a look, this is RFAL.

I've gone ahead and deprecated AdminScatter's Range.

scatter.go now constructs AdminScatterResponse_DeprecatedRange based on the RangeInfos if available with a TODO to rework that section to use RangeInfos in 21.1.
I considered requesting the RangeInfos on the batch response header directly (à la RangeStatsResponse) and ripping out DeprecatedRange from scatter.go all together. In the end I opted for this approach since using ReturnRangeInfos isn't a desirable pattern either, and we'd need to keep DeprecatedRange around anyway for backwards compatibility.

Please let me know if you prefer that approach over this one though.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @dt)

---

pkg/roachpb/api.proto, line 1549 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

call this leaseholder.

But let's do this for real. Let's deprecate this message Range, and return a repeated RangeInfo - which is a descriptor and a lease. r.adminScatter can use r.GetDescAndLease() to get a consistent view of both a descriptor and a lease.

Done.

[andreimatei]

This patch doesn't actually use the returned leases, does it?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @dt, and @pbardea)

---

pkg/roachpb/api.proto, line 1546 at r2 (raw file):

  message DeprecatedRange {

it's weird to "deprecate" a struct. I'd leave the message called Range and just rename the field to deprecated_ranges.

---

pkg/sql/scatter.go, line 125 at r2 (raw file):

	rangeIdx int
	ranges   []roachpb.AdminScatterResponse_DeprecatedRange

make this roachpb.Span

---

pkg/sql/scatter.go, line 140 at r2 (raw file):

	scatterRes := res.(*roachpb.AdminScatterResponse)
	n.run.rangeIdx = -1
	if scatterRes.RangeInfos != nil {

I'm not sure if this nil check works or if the proto decoder will always allocate a 0-len slice.

---

pkg/sql/scatter.go, line 142 at r2 (raw file):

	if scatterRes.RangeInfos != nil {
		// TODO(pbardea): In 21.1, remove all references to DeprecatedRange.
		// RangeInfos should also not be nullable then.

I see that you've already made RangeInfos not nullable - in the sense that there's no *[]Range - so this comment is confusing. I'd remove this TODO; the compatibility note below is enough.

---

pkg/sql/scatter.go, line 152 at r2 (raw file):

			}
		}
	} else {

is this correct? Given how we combine responses, you can get a response with both RangeInfos and DeprecatedRanges. I think DeprecatedRanges will always be a superset of RangeInfos. So I think you want to dedupe and use both. Or, given that you're not using the lease, just use DeprecatedRanges. Or, better yet, make the combine() always populate RangeInfos (use empty leases for the info coming from DeprecatedRanges) and wipe DeprecatedRanges such that above the DistSender one either gets only RangeInfos (when responses were combined or non-combined response from 21.1) or only DeprecatedRanges (non-combined response coming from 20.1) or both as exact duplicates (non-combined response coming from 20.2).

We can also investigate adding a compatibility response translator in the DistSender itself if you feel like it.

[pbardea]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @dt)

---

pkg/roachpb/api.proto, line 1546 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

it's weird to "deprecate" a struct. I'd leave the message called Range and just rename the field to deprecated_ranges.

Done.

---

pkg/sql/scatter.go, line 125 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

make this roachpb.Span

Done. That's much nicer - thanks.

---

pkg/sql/scatter.go, line 140 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I'm not sure if this nil check works or if the proto decoder will always allocate a 0-len slice.

Done.

---

pkg/sql/scatter.go, line 142 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I see that you've already made RangeInfos not nullable - in the sense that there's no *[]Range - so this comment is confusing. I'd remove this TODO; the compatibility note below is enough.

Done.

---

pkg/sql/scatter.go, line 152 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

is this correct? Given how we combine responses, you can get a response with both RangeInfos and DeprecatedRanges. I think DeprecatedRanges will always be a superset of RangeInfos. So I think you want to dedupe and use both. Or, given that you're not using the lease, just use DeprecatedRanges. Or, better yet, make the combine() always populate RangeInfos (use empty leases for the info coming from DeprecatedRanges) and wipe DeprecatedRanges such that above the DistSender one either gets only RangeInfos (when responses were combined or non-combined response from 21.1) or only DeprecatedRanges (non-combined response coming from 20.1) or both as exact duplicates (non-combined response coming from 20.2).

We can also investigate adding a compatibility response translator in the DistSender itself if you feel like it.

I've updated combine to always populate RangeInfos if it's empty. My original concern was that the range descriptor's range ID would not be set since the DeprecatedRanges only contain span info. Is that an issue?

[andreimatei]

LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @dt, and @pbardea)

---

pkg/sql/scatter.go, line 152 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

I've updated combine to always populate RangeInfos if it's empty. My original concern was that the range descriptor's range ID would not be set since the DeprecatedRanges only contain span info. Is that an issue?

well, hopefully nobody else will use RangeInfos until 20.2 is out

[pbardea]

TFTR!
bors r+

[pbardea]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)