@tbg reviewed 18 files and all commit messages, and made 16 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained.

-- commits line 182 at r6:
Can you update the commit message to contrast this with how the legacy allocator works and to highlight any differences?
Interactively let me engage first before pushing the suggested update.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 306 at r5 (raw file):

) []ExternalRangeChange {
	re.mmaid++
	ctx = logtags.AddTag(ctx, "mmaid", re.mmaid)

now we have more than one place that does this. Couldn't this be lifted to the caller so that we have a ctx that both repair and rebalance can use that already has the mmaid in it? Do this as a new commit in the end (i.e won't be squashed).

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 309 at r5 (raw file):

	// Iterate repair actions in priority order (lower enum = higher priority).
	for action := FinalizeAtomicReplicationChange; action < NoRepairNeeded; action++ {

• can you start at 1 (so that we don't think there's anything special about FinalizeAtomicReplicationChange)
• mention that 0 is not a valid action by design, in a comment (so nobody wonders if there's an off by one here)
• make a const numActions and make sure it doesn't rot. One way to do this is a unit test that validates that Action(numActions) still stringifies properly but numActions+1 doesn't. You might be able to think of a better way too.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 314 at r5 (raw file):

			continue
		}
		// Sort range IDs for deterministic iteration order.

See #165284 - this isn't merged but add a sync.Pool for this following that pattern regardless, so that we don't allocate slices in the common case.

I'm also worried by this determinism. We don't attempt to be entirely random, but we should also not be entirely not random. Keep the sorting, but then make range ids { iteration order deterministically random (using the rng on rebalanceEnv).

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 298 at r6 (raw file):

// replicaStateForStore returns the ReplicaState of the replica on the given
// store, and whether it was found.
func replicaStateForStore(rs *rangeState, storeID roachpb.StoreID) (ReplicaState, bool) {

can you make this a method on rangeState (and pick a good location)?

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 318 at r6 (raw file):

}

// filterAddCandidates filters candidateStores down to stores that are ready

For performance reasons, I would like the returned storeSet to be backed by the same memory as the incoming candidateStores set. This should be part of the commented contract and the caller needs to be careful when using pooled memory. See #165284 for examples of this pattern.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 322 at r6 (raw file):

// range at the node level. excludeStoreID, if non-zero, is excluded from the
// existing-replica set (used when a replica on that store is being
// concurrently removed as part of the same change).

add: , such as during non-voter promotions

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 374 at r6 (raw file):

// for diversity scoring: voterLocalityTiers for voter operations,
// replicaLocalityTiers for non-voter operations.
func (re *rebalanceEnv) pickStoreByDiversity(

Does rebalancing (not repair) have an equivalent of this or does it not care about diversity yet?
This is a discussion item.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 404 at r6 (raw file):

// repairAddVoter attempts to add a voter to an under-replicated range.
// It follows the decision tree from constraint.go: first try to promote a

the reference to constraint.go isn't going to age well, remove it.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 441 at r6 (raw file):

	re.constraintMatcher.constrainStoresForExpr(constrDisj, &candidateStores)

	validCandidates := re.filterAddCandidates(ctx, rs, candidateStores, 0)

use a local const for the 0.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 443 at r6 (raw file):

	validCandidates := re.filterAddCandidates(ctx, rs, candidateStores, 0)
	if len(validCandidates) == 0 {
		log.KvDistribution.Warningf(ctx,

Is this really a warning? I'd see this as a verbosity 1 Infof. This can happen routinely, and to many ranges, during outages or when constraints are misconfigured. Yes, worth changing, but hardly worth logging at high density or Warning.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 465 at r6 (raw file):

	rangeChange := MakePendingRangeChange(rangeID, []ReplicaChange{addChange})
	if err := re.preCheckOnApplyReplicaChanges(rangeChange); err != nil {
		log.KvDistribution.Warningf(ctx,

we don't expect to hit this, right? Then Warningf is ok but please check that this can't be routinely hit (and possibly for many ranges in one go).

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 470 at r6 (raw file):

	}
	re.enactRepair(ctx, localStoreID, rangeChange)
	log.KvDistribution.Infof(ctx,

we don't want to log at Info for routine stuff except in the aggregate. Put this behind verbosity 1.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 490 at r6 (raw file):

		(*existingReplicaLocalities).getScoreChangeForNewReplica)
	if bestStoreID == 0 {
		log.KvDistribution.Warningf(ctx,

ditto

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 498 at r6 (raw file):

	prevState, found := replicaStateForStore(rs, bestStoreID)
	if !found {
		log.KvDistribution.Warningf(ctx,

This can remain a warning, right? Since we went down this path and so there really ought to be a non-voter we could promote and in particular its store should be known?

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 522 at r6 (raw file):

	}
	re.enactRepair(ctx, localStoreID, rangeChange)
	log.KvDistribution.Infof(ctx,

ditto

Review of commits after "START REVIEWING HERE"

Two commits add repair orchestration and the first concrete repair action (AddVoter with non-voter promotion) to the MMA prototype. The code is well-structured, the commit split is clean, and the core algorithms (reservoir sampling, priority iteration, constraint filtering) are correct.

Verified correct:

• Concurrency: repair() runs under a.mu.Lock() via ComputeChanges. No concurrent access issues.
• Map mutation during iteration: repair() snapshots range IDs into ids before iterating, so enactRepair -> addPendingRangeChange -> updateRepairAction mutations are safe.
• Repair-rebalance interaction: repair pending changes correctly prevent rebalancer from touching the same ranges.
• Reservoir sampling in pickStoreByDiversity is correct standard k=1 reservoir sampling.

Strengths:

• Clean two-commit structure: orchestration skeleton first, concrete action second.
• diversityScorer function type is a clean abstraction using idiomatic Go method value expressions.
• Non-voter promotion avoids unnecessary data movement.
• Thorough multi-round DSL tests demonstrating the full repair lifecycle.

Test coverage gaps (not blocking, but worth adding):

• Leaseholder-only filtering in repair() is untested — both test files always call repair store-id=1 which is the leaseholder. Add a test where the local store is not the leaseholder to verify skipping.
• No tests for error/edge cases: no valid candidates, constraint analysis failure, pre-check failure.
• Diversity scoring with clearly unequal candidates not tested (only tied or single-candidate scenarios).

Other notes:

• The mmaid field comment on rebalanceEnv says "a counter for rebalanceStores calls" but repair() now also increments it. Worth updating.
• The repair() return value is discarded at allocator_state.go:349 since rebalanceStores() returns the same accumulated re.changes. Consider removing the return or adding a comment.

(made with /review-crdb)

@tbg reviewed 35 files and all commit messages, made 3 comments, and resolved 16 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained.

-- commits line 243 at r15:
Oh. Well I like this mmaid change. But I don't like the churn in the testdata files. Can you update the datadriven DSL directive so that it maintains an mmaid counter in the context separately in the test? Basically, make the testdata diff go away. Add a comment explaining why this is currently non-unified.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 255 at r13 (raw file):

	}
	// Remove from old bucket.
	if oldAction != NoRepairNeeded && oldAction != RepairPending && oldAction != 0 {

The contract about what is or isn't in repairRanges is a little obfuscated. Looks like RepairPending and RepairSkipped are special in that they aren't tracked. But why not, wouldn't that be more consistent and also more helpful for further metrics (not in this PR), where we will want to expose a gauge that has the count by state? I think we should explicitly track all states, except NoRepairNeeded.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 555 at r13 (raw file):

		// Collect and sort range IDs, then shuffle deterministically so that
		// iteration is not systematically biased toward any range.
		idsPtr := rangeIDSlicePool.Get().(*[]roachpb.RangeID)

can you lift this pool outside the for loop, use defer to return the slices to the pool, and reset the slices at the top of each loop?

@tbg reviewed 21 files and all commit messages, made 2 comments, and resolved 3 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 569 at r16 (raw file):

			ids[i], ids[j] = ids[j], ids[i]
		})
		*idsPtr = ids // preserve any grown capacity for next iteration

this pattern seems cleaner:

idsPtr := ...
ids := (*idsPtr)[:0]
defer func() {
  // Preserve any grown capacity for next iteration.
  ids = ids[:0]
  *idsPtr = ids
  rangeIDSlicePool.Put(idsPtr)
}()
for ... {
  ids = ids[:0]

  ids = append(ids, ...)
}

WDYT?

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_repair.go line 581 at r16 (raw file):

			case AddVoter:
				re.repairAddVoter(ctx, localStoreID, rangeID, rs)
			case RepairSkipped, RepairPending:

can you move this into the len(ranges) == 0 check at the top so we don't needlessly iterate over all the individual ranges in cases where we're going to not act on any?

@tbg reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on tbg).

@tbg reviewed 10 files and all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on tbg).