Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: let per-replica constraints specify less than all a zone's replicas #23057

Merged
merged 1 commit into from
Feb 26, 2018

Conversation

a-robinson
Copy link
Contributor

Release note (cli change): Per-replica constraints no longer have to add
up to the total number of replicas in a range. If don't specify all the
replicas, then the remaining replicas will be allowed on any store.

Release note (cli change): Per-replica constraints no longer have to add
up to the total number of replicas in a range. If don't specify all the
replicas, then the remaining replicas will be allowed on any store.
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis
Copy link
Collaborator

:lgtm:

The complaint about the difficulty in understanding the test cases shouldn't hold up this PR. I did work through what they are testing and they look correct.


Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/storage/allocator_scorer_test.go, line 746 at r1 (raw file):

		},
		{
			name: "multiple required constraints with NumReplicas and sum(NumReplicas) < zone.NumReplicas",

I'm going to reiterate my complaint about these tests. They are quite difficult to review because the context needed to review them is not local. I had to open up allocator_scorer_test.go in an editor and flip back and forth looking at the testStores setup. I don't have a concrete suggestion, only a feeling that something can be improved here.


Comments from Reviewable

@a-robinson
Copy link
Contributor Author

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/storage/allocator_scorer_test.go, line 746 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I'm going to reiterate my complaint about these tests. They are quite difficult to review because the context needed to review them is not local. I had to open up allocator_scorer_test.go in an editor and flip back and forth looking at the testStores setup. I don't have a concrete suggestion, only a feeling that something can be improved here.

I assume some better naming of the stores (in these tests) or using names at all rather than just numbers (in the big rebalance tests) would help.


Comments from Reviewable

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.

None yet

3 participants