Merge #158333

158333: mmaprototype: small clean up doStructuralNormalization r=wenyihu6 a=wenyihu6

Epic: CRDB-55052 
Release note: none

---
**mmaprototype: add assertion on emptyVoterConstraintIndex**

This commit adds assertions on emptyVoterConstraintIndex and
emptyConstraintIndex since there should only be one.

---
**mmaprototype: clean up doStructuralNormalization**

This commit refactors some code in doStructuralNormalization to
use min utility and also adds assertions on programming errors.

---
**mmaprototype: use >= 0 for emptyVoterConstraintIndex**

This commit fixes >= 0 when checking for whether there were
valid emptyVoterConstraintIndex and emptyConstraintIndex
since 0 is a valid index.

Co-authored-by: wenyihu6 <[email protected]>

Author: craig[bot]

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

@@ -474,15 +474,37 @@ func doStructuralNormalization(conf *normalizedSpanConfig) error {
 		allIndex       int
 		voterAndAllRel conjunctionRelationship
 	}
+	// emptyConstraintIndex corresponds to the index of the empty constraint in
+	// conf.constraints. We expect only one empty constraint.
+	// Example:
+	// constraints: []: 2, [+region=a]: 2 => emptyConstraintIndex = 0
+	//
+	// TODO(wenyihu6): we should not allow user to specify empty constraints;
+	// there should only be one created by us during normalizeConstraints.
+	// Instead, we can also combine empty constraints into a single one in
+	// normalizeConstraints.
 	emptyConstraintIndex := -1
+	// emptyVoterConstraintIndex corresponds to the index of the empty voter
+	// constraint in conf.voterConstraints. We expect only one empty voter
+	// constraint.
+	// Example:
+	// voterConstraints: [+region=a]: 2, []: 2 => emptyVoterConstraintIndex = 1
 	emptyVoterConstraintIndex := -1
 	var rels []relationshipVoterAndAll
 	for i := range conf.voterConstraints {
 		if len(conf.voterConstraints[i].constraints) == 0 {
+			if emptyVoterConstraintIndex != -1 {
+				return errors.Errorf("multiple empty voter constraints: %v and %v",
+					conf.voterConstraints[emptyVoterConstraintIndex], conf.voterConstraints[i])
+			}
 			emptyVoterConstraintIndex = i
 		}
 		for j := range conf.constraints {
 			if len(conf.constraints[j].constraints) == 0 {
+				if emptyConstraintIndex != -1 && emptyConstraintIndex != j {
+					return errors.Errorf("multiple empty constraints: %v and %v",
+						conf.constraints[emptyConstraintIndex], conf.constraints[j])
+				}
 				emptyConstraintIndex = j
 			}
 			rels = append(rels, relationshipVoterAndAll{
@@ -577,10 +599,7 @@ func doStructuralNormalization(conf *normalizedSpanConfig) error {
 			voterConstraints[rel.voterIndex].numReplicas
 		if neededVoterReplicas > 0 && remainingAll > 0 {
 			// We can satisfy some voter replicas.
-			toAdd := remainingAll
-			if toAdd > neededVoterReplicas {
-				toAdd = neededVoterReplicas
-			}
+			toAdd := min(remainingAll, neededVoterReplicas)
 			voterConstraints[rel.voterIndex].numReplicas += toAdd
 			allReplicaConstraints[rel.allIndex].remainingReplicas -= toAdd
 		}
@@ -602,20 +621,20 @@ func doStructuralNormalization(conf *normalizedSpanConfig) error {
 	// has some remainingReplicas, we take them here to satisfy the empty voter
 	// constraint as much as possible. Only what is remaining in the empty voter
 	// constraint will then be available for subsequent narrowing.
-	if emptyVoterConstraintIndex > 0 && emptyConstraintIndex > 0 {
+	if emptyVoterConstraintIndex >= 0 && emptyConstraintIndex >= 0 {
 		neededReplicas := conf.voterConstraints[emptyVoterConstraintIndex].numReplicas
-		actualReplicas := voterConstraints[emptyVoterConstraintIndex].numReplicas
-		remaining := neededReplicas - actualReplicas
-		if remaining > 0 {
-			remainingSatisfiable := allReplicaConstraints[emptyConstraintIndex].remainingReplicas
-			if remainingSatisfiable > 0 {
-				count := remainingSatisfiable
-				if count > remaining {
-					count = remaining
-				}
-				voterConstraints[emptyVoterConstraintIndex].numReplicas += count
-				allReplicaConstraints[emptyConstraintIndex].remainingReplicas -= count
-			}
+		// While iterating over the previous relationships, we skipped over
+		// emptyVoterConstraintIndex, so its corresponding
+		// voterConstraints.numReplicas must be 0.
+		if voterConstraints[emptyVoterConstraintIndex].numReplicas != 0 {
+			panic(errors.AssertionFailedf("programming error: voterConstraints[%d].numReplicas should be 0, but is %d",
+				emptyVoterConstraintIndex, voterConstraints[emptyVoterConstraintIndex]))
+		}
+		remainingSatisfiable := allReplicaConstraints[emptyConstraintIndex].remainingReplicas
+		if neededReplicas > 0 && remainingSatisfiable > 0 {
+			count := min(remainingSatisfiable, neededReplicas)
+			voterConstraints[emptyVoterConstraintIndex].numReplicas += count
+			allReplicaConstraints[emptyConstraintIndex].remainingReplicas -= count
 		}
 	}
 
@@ -761,19 +780,17 @@ func doStructuralNormalization(conf *normalizedSpanConfig) error {
 				// rel.voterIndex must be emptyVoterConstraintIndex.
 				continue
 			}
-			if conf.constraints[rel.allIndex].numReplicas < conf.voterConstraints[rel.voterIndex].numReplicas {
-				toAddCount := conf.voterConstraints[rel.voterIndex].numReplicas -
-					conf.constraints[rel.allIndex].numReplicas
-				availableCount := conf.constraints[emptyConstraintIndex].numReplicas
-				if availableCount < toAddCount {
-					// TODO(wenyihu6): this should also create an error, since we will
-					// end up with two identical constraint conjunctions in replica
-					// constraints and voter constraints, with the former having a lower
-					// count than the latter.
-					toAddCount = availableCount
-				}
-				conf.constraints[emptyConstraintIndex].numReplicas -= toAddCount
-				conf.constraints[rel.allIndex].numReplicas += toAddCount
+			toAddCount := conf.voterConstraints[rel.voterIndex].numReplicas -
+				conf.constraints[rel.allIndex].numReplicas
+			availableCount := conf.constraints[emptyConstraintIndex].numReplicas
+			if toAddCount > 0 && availableCount > 0 {
+				// TODO(wenyihu6): this should also create an error, since we will
+				// end up with two identical constraint conjunctions in replica
+				// constraints and voter constraints, with the former having a lower
+				// count than the latter.
+				add := min(toAddCount, availableCount)
+				conf.constraints[emptyConstraintIndex].numReplicas -= add
+				conf.constraints[rel.allIndex].numReplicas += add
 			}
 		}
 		// For conjStrictSubset, if the subset relationship is with
@@ -790,14 +807,12 @@ func doStructuralNormalization(conf *normalizedSpanConfig) error {
 				continue
 			}
 			availableCount := conf.constraints[emptyConstraintIndex].numReplicas
-			if availableCount > 0 {
-				toAddCount := conf.voterConstraints[rel.voterIndex].numReplicas
-				if toAddCount > availableCount {
-					toAddCount = availableCount
-				}
-				conf.constraints[emptyConstraintIndex].numReplicas -= toAddCount
+			toAddCount := conf.voterConstraints[rel.voterIndex].numReplicas
+			if availableCount > 0 && toAddCount > 0 {
+				add := min(availableCount, toAddCount)
+				conf.constraints[emptyConstraintIndex].numReplicas -= add
 				conf.constraints = append(conf.constraints, internedConstraintsConjunction{
-					numReplicas: toAddCount,
+					numReplicas: add,
 					constraints: conf.voterConstraints[rel.voterIndex].constraints,
 				})
 			}

pkg/kv/kvserver/allocator/mmaprototype/testdata/normalize_config

@@ -609,8 +609,6 @@ output:
 
 # Replicas are not constrained. The voter constraints are promoted to replica
 # constraints.
-#
-# TODO(sumeer): investigate why the normalization also threw an error.
 normalize num-replicas=5 num-voters=3
 constraint num-replicas=5
 voter-constraint num-replicas=1 +region=a +zone=a1
@@ -623,7 +621,6 @@ input:
  voter-constraints:
    +region=a,+zone=a1:1
    +region=a,+zone=a2:1
-err=could not satisfy all voter constraints due to non-intersecting conjunctions in voter and all replica constraints
 output:
  num-replicas=5 num-voters=3
  constraints: