kvserver: add load vec by kvoli · Pull Request #91593 · cockroachdb/cockroach

kvoli · 2022-11-09T13:20:08Z

Previously, disparate types were used when comparing StoreCapacity,
XThreshold and RangeUsageInfo. This patch introduces a uniform
intermediate type Load, which is used to perform arithmetic and
comparison between types representing load.

The purpose of this change is to decouple changes to inputs, enable
modifying existing dimensions and adding new ones with less code
modification.

The only load dimension added in this patch is Queries, which is then
used in place of QueriesPerSecond within the rebalancing logic.

Additionally, RangeUsageInfo is uniformly passed around in place of
any specific calls to the underlying tracking datastructure
ReplicaStats.

Part of #91152

Release note: None

cockroach-teamcity · 2022-11-09T13:20:17Z

This change is

andrewbaptist

This mostly looks great. Most of the comments are pretty minor, but I'm a little confused about some of the "multi-dimension" code that exists.

Reviewed 37 of 37 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @kvoli and @nvanbenschoten)

pkg/kv/kvserver/replica_rankings.go line 103 at r1 (raw file):

//
// TODO(kvoli): When adding another load dimension to be balanced upon, it will
// be necessary to clairfy the semantics of this API. This is especially true

nit: clairfy -> clarify

pkg/kv/kvserver/replica_rankings.go line 105 at r1 (raw file):

// be necessary to clairfy the semantics of this API. This is especially true
// since the UI is coupled to this function.
func (rr *ReplicaRankings) NewAccumulator(dimension load.Dimension) *RRAccumulator {

I'm confused why this is a method on ReplicaRankings instead of just a function. You don't use the rr in here at all. It looks like it was this way before, but makes it a little more confusing to read.

pkg/kv/kvserver/replica_rankings.go line 223 at r1 (raw file):

// TopQPS returns the highest QPS CandidateReplicas that are tracked.
func (rr *ReplicaRankingMap) TopQPS(tenantID roachpb.TenantID) []CandidateReplica {

Rename to TopLoad?

pkg/kv/kvserver/replica_rankings.go line 244 at r1 (raw file):

func (a RRAccumulatorByTenant) AddReplica(repl CandidateReplica) {
	// Do not consider ranges as hot when they are accessed once or less times.
	if repl.RangeUsageInfo().QueriesPerSecond <= 1 {

I probably just need a walk through with you on this. But I assumed that the function here would be "generic" over the replica. Here it is still assuming QPS.

pkg/kv/kvserver/replicate_queue.go line 2167 at r1 (raw file):

// NB: This is currently just the replicas usage, it assumes that the range's
// usage information matches. Which is dubious in some cases but often
// reasoanble when we only consider the leaseholder.

nit: reasoanble -> reasonable

pkg/kv/kvserver/store.go line 3411 at r1 (raw file):

// out of date.
func (s *Store) HottestReplicas() []HotReplicaInfo {
	topQPS := s.replRankings.TopLoad()

nit: rename variable.

pkg/kv/kvserver/store_rebalancer.go line 714 at r1 (raw file):

		desc, conf := candidateReplica.DescAndSpanConfig()
		log.KvDistribution.VEventf(ctx, 3, "considering lease transfer for r%d with %s load",

Consider printing the dimension being used here so that the "units" make sense. Otherwise, this might be hard to debug if the configuration isn't known. (add on other events as well).

pkg/kv/kvserver/store_rebalancer.go line 824 at r1 (raw file):

		if candidateReplica == nil {
			panic("programming error: nil candidate replica found")

This is unnecessary to have this check at all anymore. It will panic anyway on the line below.

pkg/kv/kvserver/allocator/base.go line 176 at r1 (raw file):

	// LoadDimensions declares the load dimensions to use when the Goal is
	// LoadConvergence.
	LoadDimensions []load.Dimension

I'm a little confused why this is an array (rather than just a single Dimension). I'm not clear that this is needed here at all since it can be pulled from configuration when needed (should discuss at walkthrough).

pkg/kv/kvserver/allocator/allocatorimpl/allocator.go line 1923 at r1 (raw file):

				LoadThreshold:                LoadThresholds(&a.st.SV, opts.LoadDimensions...),
				MinLoadThreshold:             LoadMinThresholds(opts.LoadDimensions...),
				MinRequiredRebalanceLoadDiff: LoadRebalanceRequiredMinDiff(&a.st.SV, opts.LoadDimensions...),

It seems like this would be much better if it was a ratio rather than an absolute difference.

pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go line 417 at r1 (raw file):

	// MinRequiredRebalanceLoadDiff declares the minimum load difference
	// between stores required before recommending an action to rebalance.
	MinRequiredRebalanceLoadDiff load.Load

As noted elsewhere, can this be changed to be a ratio rather than an absolute difference?

pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go line 438 at r1 (raw file):

	// rebalance or transfer. These shoulld more generally use two different
	// methods when estimating an impact.
	RebalanceImpact load.Load

This seems very difficult to calculate correctly. Also, I'm a little confused if you are computing the "cost" of follower replicas. If not, then it may skew the results badly. Let's discuss during the walkthrough.

pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go line 496 at r1 (raw file):

		log.KvDistribution.VEventf(
			ctx, 4,
			"should rebalance replica with %0.2f qps from s%d (qps=%0.2f) to s%d (qps=%0.2f)",

Fix the comment to not say qps

pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go line 1205 at r1 (raw file):

			len(options.LoadDims)))
	}
	dimension := options.LoadDims[0]

Should LoadDims just be a singleton rather than an array?

pkg/kv/kvserver/allocator/load/dimension.go line 22 at r1 (raw file):

const (
	nDimensions = 1

nit: This is better done with
`nDimensions int = int(Queries) + 1 put at the end of the const block above. This makes it less likely to be missed when new dimensions are added.

pkg/kv/kvserver/allocator/load/load.go line 36 at r1 (raw file):

	Min(other Load) Load
	// Mul multiplies the calling Load with other and returns the result.
	Mul(other Load) Load

Mul seems strange. Its not clear what the units would be on this anymore... For instance if it was Q/S before it would now be Q^2/S^2. This would be clearer if it was Mul(threshold float).

pkg/kv/kvserver/asim/storerebalancer/replica_rankings.go line 21 at r1 (raw file):

func hottestRanges(state state.State, storeID state.StoreID) []kvserver.CandidateReplica {
	replRankings := kvserver.NewReplicaRankings()
	accumulator := replRankings.NewAccumulator(load.Queries)

Shouldn't the dimension be passed in here and used instead?

pkg/kv/kvserver/replicastats/replica_stats.go line 397 at r1 (raw file):

func (rs *ReplicaStats) SnapshotRatedSummary() *RatedSummary {
	qps, duration := rs.AverageRatePerSecond()
	localityCounts, _ := rs.PerLocalityDecayingRate()

I don't understand the significance of this change. Minor nit is that we should change the signature of PerLocalityDecayingRate to not return the duration anymore as this is the only production use of it.

pkg/kv/kvserver/replica_rankings_test.go line 83 at r1 (raw file):

		replsCopy := rr.TopLoad()
		if !reflect.DeepEqual(repls, replsCopy) {
			t.Errorf("got different replicas on second call to topQPS; first call: %v, second call: %v", repls, replsCopy)

nit: topQPS -> TopLoad

kvoli

Thanks for taking a look! Lets discuss the multiple dimension part - it is done with an intent to use more than one dimension of load at a time in the near future.

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

pkg/kv/kvserver/replica_rankings.go line 103 at r1 (raw file):