Skip to content

Conversation

@wenyihu6
Copy link
Contributor

@wenyihu6 wenyihu6 commented Nov 26, 2025

Epic: CRDB-55052
Release note: none


mmaprototype: extract relationship building between voter and all-replica constraints

@wenyihu6 wenyihu6 requested review from a team as code owners November 26, 2025 18:27
@wenyihu6 wenyihu6 requested review from sumeerbhola and tbg November 26, 2025 18:27
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sumeerbhola reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @tbg)


pkg/kv/kvserver/allocator/mmaprototype/testdata/voter_and_all_relationship_table line 112 at r1 (raw file):


# Test error case: constraint replicas add up to more than configured replicas.
voter-and-all-relationship num-replicas=1 num-voters=1

So this is a test for makeBasicNormalizedSpanConfig too? If yes, consider renaming the test.


pkg/kv/kvserver/allocator/mmaprototype/testdata/voter_and_all_relationship_table line 150 at r1 (raw file):

 constraints:
   +region=us-west:2
   +region=us-east,+zone=us-east-a:2

I think the test should output the config after normalization, since we need to see those to understand that the added empty constraints are in the cross-product.


pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 456 at r1 (raw file):

		if len(conf.voterConstraints[i].constraints) == 0 {
			if emptyVoterConstraintIndex != -1 {
				return nil, -1, -1, errors.Errorf("invalid configurations with empty voter constraint")

... with multiple empty ...

Could you add a TODO here to change this error to a panic once we fix normalizeConstraints to de-dup. This error is not the fault of the user.


pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 463 at r1 (raw file):

			if len(conf.constraints[j].constraints) == 0 {
				if emptyConstraintIndex != -1 && emptyConstraintIndex != j {
					return nil, -1, -1, errors.Errorf("invalid configurations with empty constraint")

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants