mmaprototype: improve comments and logs

This commit resolves misc comments from PR review.

Author: wenyihu6

pkg/kv/kvserver/allocator/mmaprototype/rebalance_advisor.go

@@ -13,8 +13,9 @@ import (
 )
 
 // MMARebalanceAdvisor contains information that mma needs to determine if a
-// candidate is in conflict with its goals. All fields should be immutable after
-// its initialization.
+// candidate is in conflict with its goals. The caller should not access/change
+// its internal fields after creation and should use IsInConflictWithMMA
+// instead.
 //
 // MMARebalanceAdvisor uses the meansLoad summary to compute the load summary
 // for a provided candidate. Then it compares the candidate load summary against
@@ -47,11 +48,32 @@ func NoopMMARebalanceAdvisor() *MMARebalanceAdvisor {
 
 // BuildMMARebalanceAdvisor constructs an MMARebalanceAdvisor for the given
 // existing store and candidate stores. The advisor can be used to determine if
-// a candidate is in conflict with the existing store via IsInConflictWithMMA.
-// The provided cands list may or may not include the existing store. This
-// method always adds the existing store to the cands list so that it is
-// included in the mean calculation. It is up to computeMeansForStoreSet to
-// handle the de-duplication of storeIDs from the cands list.
+// a candidate is vetoed by the multi-metric allocator due to running counter to
+// its goals. Currently, it is considered counter to its goal if the candidate
+// is more overloaded than the existing store with respect to the cands list.
+//
+// The provided cands list may or may not include the existing store. For the
+// replicate queue, the cands list is just the set of candidate stores for the
+// equivalence class, excluding the existing store. For the lease queue, the
+// cands list includes all stores that satisfy the constraint including the
+// existing store.
+//
+// This method always adds the existing store to the cands list to ensure it is
+// included in the mean calculation. This is subject to change, but currently it
+// does not make sense to compute the load summary of the existing store with
+// respect to the cands list that does not include the existing store itself.
+// Alternatively, we could require the cands list to be exactly the same as the
+// set of stores used to compute the means for lease / range count. The existing
+// store should be included for the lease queue but excluded for the replicate
+// queue. Note that since this method always adds the existing store to the
+// list, and the provided cands may or may not already include the existing
+// store, the cands slice passed to computeMeansForStoreSet may contain
+// duplicate storeIDs. It is up to computeMeansForStoreSet to handle
+// de-duplication of storeIDs from the cands list.
+//
+// The returned advisor should be passed to IsInConflictWithMMA as a helper to
+// determine if a candidate is vetoed by the multi-metric allocator due to
+// running counter to its goals.
 func (a *allocatorState) BuildMMARebalanceAdvisor(
 	existing roachpb.StoreID, cands []roachpb.StoreID,
 ) *MMARebalanceAdvisor {
@@ -70,8 +92,12 @@ func (a *allocatorState) BuildMMARebalanceAdvisor(
 // IsInConflictWithMMA determines if the given candidate is in conflict with the
 // existing store using the provided MMARebalanceAdvisor. For simplicity, we
 // currently say that this is in conflict if the candidate is more overloaded
-// than the existing store. This is subject to change in the future. Caller is
-// responsible for making sure the MMARebalanceAdvisor is for the correct
+// than the existing store. This is subject to change in the future. When
+// cpuOnly is true, the candidate store's cpu load summary must not exceed that
+// of the existing store, or the operation is considered in conflict. When
+// cpuOnly is false, the worst dimension's summary is compared instead.
+//
+// Caller is responsible for making sure the MMARebalanceAdvisor is for the correct
 // existing store and candidate set.
 func (a *allocatorState) IsInConflictWithMMA(
 	ctx context.Context, cand roachpb.StoreID, advisor *MMARebalanceAdvisor, cpuOnly bool,
@@ -88,13 +114,14 @@ func (a *allocatorState) IsInConflictWithMMA(
 	// Always compute the candidate's load summary.
 	candSLS := a.cs.computeLoadSummary(ctx, cand, &advisor.means.storeLoad, &advisor.means.nodeLoad)
 
+	// TODO(wenyihu6): unify the branches below by assigning based on sls.worstDim and cpuOnly.
 	var conflict bool
 	if cpuOnly {
 		conflict = candSLS.dimSummary[CPURate] > existingSLS.dimSummary[CPURate]
 		if conflict {
 			log.KvDistribution.VEventf(
 				ctx, 2,
-				"mma rejected candidate s%d(cpu-only) as a replacement for s%d: candidate=%v > existing=%v",
+				"mma rejected candidate s%d(cpu-only) as a replacement for s%d: candidate=%v(cpu) > existing=%v(cpu)",
 				cand, advisor.existingStoreID, candSLS.dimSummary[CPURate], existingSLS.dimSummary[CPURate],
 			)
 		}
@@ -103,8 +130,8 @@ func (a *allocatorState) IsInConflictWithMMA(
 		if conflict {
 			log.KvDistribution.VEventf(
 				ctx, 2,
-				"mma rejected candidate s%d as a replacement for s%d: candidate=%v > existing=%v",
-				cand, advisor.existingStoreID, candSLS.sls, existingSLS.sls,
+				"mma rejected candidate s%d as a replacement for s%d: candidate=%v(%v) > existing=%v(%v)",
+				cand, advisor.existingStoreID, candSLS.sls, candSLS.worstDim, existingSLS.sls, existingSLS.worstDim,
 			)
 		}
 	}

pkg/kv/kvserver/mmaintegration/thrashing.go

@@ -68,8 +68,11 @@ import (
 // stores that satisfy the constraint to compute the lease count mean as well.
 // This approach differs from how mma computes load summary for lease transfers
 // (mma computes load summary over stores that the existing replicas are on).
-// • Note that MMARebalanceAdvisor also always include the existing store in the
-// set of stores to compute the load summary with respect to.
+// • On top of the cands list, MMARebalanceAdvisor currently also always include
+// the existing store in the set of stores to compute the load summary with
+// respect to. This is subject to change, but currently it does not make sense
+// to compute the load summary of the existing store with respect to the cands
+// list that does not include the existing store itself.
 //
 // Alternatives considered:
 // 1. Another option was to let mma choose from a set of candidates, but this was