kvserver: improve definition of equivalence classes by aayushshah15 · Pull Request #73951 · cockroachdb/cockroach

aayushshah15 · 2021-12-16T23:18:27Z

This commit makes an effort to make equivalenceClasses more well-defined.
Previously, we would coalesce the equivalence classes for any two existing
replicas if they shared the exact same locality hierarchy (including the node
ids). This logic became ineffective with #51567 since we disallow multiple
replicas on a single node under all circumstances. Furthermore, coalescing the
equivalence classes for existing replicas did not buy us anything and instead
required a bunch of custom code for us to correctly deal with them.

This commit takes an opinionated approach and gets rid of the logic that
coalesces two existing replicas' equivalence classes together.

Release note: None

cockroach-teamcity · 2021-12-16T23:18:36Z

This change is

aayushshah15 · 2021-12-16T23:18:52Z

First 3 commits are from #73597

nvb · 2021-12-19T23:13:29Z

@aayushshah15 do you mind rebasing this on master now that #73597 has landed?

aayushshah15 · 2021-12-20T14:25:29Z

Done.

nvb

nice simplification! I'm glad we weren't missing something when discussing this.

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

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

	// pretty sure we won't want to remove immediately after adding it.
	// If we would, we don't want to actually rebalance to that target.
	var target *candidate

nit: var target, existingCandidate *candidate

pkg/kv/kvserver/allocator_scorer.go, line 771 at r1 (raw file):

}

// rebalanceOptions contains two candidate lists:

"two candidate lists" is no longer accurate.

pkg/kv/kvserver/allocator_scorer.go, line 775 at r1 (raw file):

// 1. an existing replica.
// 2. a corresponding list of comparable stores that could be legal replacements
// for the aforementioned existing replicas -- also ordered from `best()` to

Remove "also".

pkg/kv/kvserver/allocator_scorer.go, line 989 at r1 (raw file):

	results := make([]rebalanceOptions, 0, len(equivalenceClasses))
	for _, comparable := range equivalenceClasses {
		var candidates candidateList

nit: move this down right about the loop that populates it.

pkg/kv/kvserver/allocator_scorer.go, line 1058 at r1 (raw file):

func bestRebalanceTarget(
	randGen allocatorRand, options []rebalanceOptions,
) (*candidate, *candidate) {

Let's give these return vars names. That also lets us omit the repeat type. ) (target, existingCandidate *candidate) {

This commit makes an effort to make `equivalenceClass`es more well-defined. Previously, we would coalesce the equivalence classes for any two existing replicas if they shared the exact same locality hierarchy (including the node ids). This logic became ineffective with cockroachdb#51567 since we disallow multiple replicas on a single node under all circumstances. Furthermore, coalescing the equivalence classes for existing replicas did not buy us anything and instead required a bunch of custom code for us to correctly deal with them. This commit takes an opinionated approach and gets rid of the logic that coalesces two existing replicas' equivalence classes together. Release note: None

aayushshah15

Thanks for the review!

bors r+

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

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