kvserver: teach replicateQueue to handle non-voter addition/removal

[aayushshah15]

This commit primarily teaches the allocator to be able to rank
non-voting replica candidates for addition and removal. This allows us
to have the replicateQueue execute upon the allocator's actions to add
or remove non-voting replicas to a range.

Note that this commit does not deal with rebalancing of non-voting
replicas, just simple additions and removals when a range is over or
under-replicated.

Release note: None

[cockroach-teamcity]

This change is 

[aayushshah15]

I'm still fixing a few broken tests, but I thought I'd open it up for review while I do that.

[nvanbenschoten]

Gave the last commit a pass. You mentioned that this is still a WIP and is based on #59389. Is this blocked on that PR?

Reviewed 1 of 7 files at r7, 10 of 10 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15 and @andreimatei)

---

pkg/kv/kvserver/allocator.go, line 544 at r8 (raw file):

}

type chooseReplicaToAddFn func(

I don't find that the indirection through chooseReplicaToAddFn or chooseReplicaToRemoveFn provides much clarity, because the corresponding functions (e.g. allocateVoterCandidates vs. allocateNonVotersFromList and even allocateVoterCandidates vs. allocateNonVoterCandidates) are almost identical. So this is leading to a lot of code duplication and it's also making it quite difficult to understand where the logic isn't identical. As a reader trying to understand this, I'd like to more easily be able to point to one or two places where this distinction matters (e.g. using constraint.EmptyAnalyzedConstraints for voter constraints when allocating a non-voting replica) and know that it doesn't in all other places.

This makes me wonder if we're splitting code paths too early. Did you explore how this code would look with a voterTarget bool parameter plumbed through a few of these layers and resorted to targetted branching on this condition?

---

pkg/kv/kvserver/allocator.go, line 763 at r8 (raw file):

// set. It first attempts to randomly select a target from the set of stores
// that have greater than the average number of replicas. Failing that, it
// falls back to selecting a random target from any of the existing

"existing voting replicas"

---

pkg/kv/kvserver/allocator.go, line 779 at r8 (raw file):

// provided set. It first attempts to randomly select a target from the set of
// stores that have greater than the average number of replicas. Failing that,
// it falls back to selecting a random target from any of the existing replicas.

"existing non-voting replicas"

---

pkg/kv/kvserver/allocator_scorer.go, line 1003 at r8 (raw file):

			valid = true
			matchingStores := analyzed.SatisfiedBy[i]
			// TODO DURING REVIEW: Not directly related to this patch, but shouldn't

Reading the comment on the function, I think the idea is that the target store is necessary if "it matches a constraint that is not already fully satisfied by existing replicas". If len(matchingStores) == int(constraints.NumReplicas) then the constraint is already satisfied without store, assuming store itself is not accounted for in analyzed.SatisfiedBy. I don't think it is, given the comment on removeConstraintsCheck, which says "The difference between this and allocateConstraintsCheck is that this is to be used on an existing replica of the range, not a potential addition.".

Mind leaving a comment?

---

pkg/kv/kvserver/replicate_queue.go, line 348 at r7 (raw file):

 /* removeIdx */

Looks like we accidentally lost this.

---

pkg/kv/kvserver/replicate_queue.go, line 429 at r7 (raw file):

	existingReplicas []roachpb.ReplicaDescriptor,
	liveVoterReplicas []roachpb.ReplicaDescriptor,
	removeIdx int, // -1 for no removal

Looks like we lost this comment.

---

pkg/kv/kvserver/replicate_queue.go, line 123 at r8 (raw file):

// ReplicateQueueMetrics is the set of metrics for the replicate queue.
// TODO(aayush): Track metrics for non-voting replicas separately here.

👍 are you planning to do that in this PR, or should we open an issue to track?

---

pkg/kv/kvserver/replicate_queue.go, line 550 at r8 (raw file):

}

func (rq *replicateQueue) addNonVoter(

Let's give this a comment.

---

pkg/kv/kvserver/replicate_queue.go, line 778 at r8 (raw file):

	}

	if err := rq.changeReplicas(ctx, repl,

nit: let's keep the wrapping the same as in all other callers, so it's easier to see where this differs from them.

[aayushshah15]

I was intending for it to not be a WIP anymore. It is blocked on #59389 though.

Your comment regarding the code duplication captures the gist of what I most wanted your opinion on. Thanks for taking a look. I'll make that change.

[aayushshah15]

TFTR!

bors r+

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

---

pkg/kv/kvserver/allocator_test.go, line 2639 at r11 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: was this intentional?

Fixed.

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)