kvserver: add no-op safeguard to TransferLeaseTarget

[aayushshah15]

kvserver: add no-op safeguard to TransferLeaseTarget

Release justification: adds no-op safeguard

Release note: None

[cockroach-teamcity]

This change is 

[andreimatei]

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

---

pkg/kv/kvserver/allocator.go, line 1257 at r1 (raw file):

// TransferLeaseTarget returns a suitable replica to transfer the range lease
// to from the provided list. It excludes the current lease holder replica
// unless asked to do otherwise by the checkTransferLeaseSource parameter.

would you mind taking the opportunity to put a comment oncheckTransferLeaseSource? I can't really tell what the deal with it is.

---

pkg/kv/kvserver/allocator.go, line 1454 at r1 (raw file):

		currentDelta := getQPSDelta(storeQPSMap, existing)
		// NB: If the `bestOption` ends up being the current leaseholder, then the
		// check below will fail since we know that `leaseholderReplQPS` must be

you say the check will fail, but won't it pass?

TBH, I can't really tell what this change is doing (btw, the commit msg says it's comments only, but it's not). So tell me the dealeo, or otherwise perhaps clarify this comment more?

[aayushshah15]

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

---

pkg/kv/kvserver/allocator.go, line 1257 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

would you mind taking the opportunity to put a comment oncheckTransferLeaseSource? I can't really tell what the deal with it is.

done, let me know if its lacking.

---

pkg/kv/kvserver/allocator.go, line 1454 at r1 (raw file):
The comment was perhaps in the wrong place. See now.

TBH, I can't really tell what this change is doing (btw, the commit msg says it's comments only, but it's not). So tell me the dealeo, or otherwise perhaps clarify this comment more?

See

cockroach/pkg/kv/kvserver/store_rebalancer.go

Lines 412 to 422 in d61f474

	// Don't bother moving leases whose QPS is below some small fraction of the
	// store's QPS (unless the store has extra leases to spare anyway). It's
	// just unnecessary churn with no benefit to move leases responsible for,
	// for example, 1 qps on a store with 5000 qps.
	const minQPSFraction = .001
	if replWithStats.qps < localDesc.Capacity.QueriesPerSecond*minQPSFraction &&
	float64(localDesc.Capacity.LeaseCount) <= storeList.candidateLeases.mean {
	log.VEventf(ctx, 3, "r%d's %.2f qps is too little to matter relative to s%d's %.2f total qps",
	replWithStats.repl.RangeID, replWithStats.qps, localDesc.StoreID, localDesc.Capacity.QueriesPerSecond)
	continue
	}

[nvanbenschoten]

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

---

pkg/kv/kvserver/allocator.go, line 1454 at r2 (raw file):

		currentDelta := getQPSDelta(storeQPSMap, existing)
		bestOption := getCandidateWithMinQPS(storeQPSMap, existing)
		if bestOption != (roachpb.ReplicaDescriptor{}) && leaseholderReplQPS > 0 &&

This looks good, although I'm left wondering why it was preferable to write like this, as opposed to the more direct

if bestOption != (roachpb.ReplicaDescriptor{}) && bestOption.StoreID != leaseRepl.StoreID()

[aayushshah15]

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

---

pkg/kv/kvserver/allocator.go, line 1454 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This looks good, although I'm left wondering why it was preferable to write like this, as opposed to the more direct

if bestOption != (roachpb.ReplicaDescriptor{}) && bestOption.StoreID != leaseRepl.StoreID()

Yeah you're right, evidently Andrei found this weird as well. Changed.

[aayushshah15]

TFTRs

bors r+

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[andreimatei]

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

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)