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

storage: update rule solver candidates to have 2 scores and a valid bool #11702

Closed
wants to merge 2 commits into from

Conversation

BramGruneir
Copy link
Member

@BramGruneir BramGruneir commented Nov 29, 2016

Do not merge until after #11695.
Ignore the first commit, it is part of #11695.

Two commits:
storage: convert rule solver rules to return 2 scores
This commit splits the scores returned in the rule solver into a constraint and a balance score.

storage: add a valid field to rule solver's candidates
Solve now returns all candidates instead of just the valid ones. To
get only the valid candidates, the new function onlyValid and new type
alias condidateList have also been added.

This change allows us to use solve for removeTarget. It also cleans
up the logic in removeTarget to more closely match the non-rule solver
version.


This change is Reviewable

@petermattis
Copy link
Collaborator

Not finished reviewing this yet, just sending out some initial comments.


Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


pkg/storage/allocator.go, line 310 at r3 (raw file):

		)

		if len(candidates) == 0 {
if len(candidates) != 0 {
  badStoreID = candidates[len(candidates)-1].store.StoreID
}

pkg/storage/allocator.go, line 322 at r3 (raw file):

			return roachpb.ReplicaDescriptor{}, errors.New("could not select an appropriate replica to be removed")
		}
		badStoreID = bad.StoreID
if bad != nil {
  badStoreID = bad.StoreID
}

pkg/storage/allocator.go, line 325 at r3 (raw file):

	}

	for _, exist := range existing {
if badStoreID != 0 {
...
}

Comments from Reviewable

@BramGruneir
Copy link
Member Author

Cool.


Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


pkg/storage/allocator.go, line 310 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…
if len(candidates) != 0 {
  badStoreID = candidates[len(candidates)-1].store.StoreID
}

Done.


pkg/storage/allocator.go, line 322 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…
if bad != nil {
  badStoreID = bad.StoreID
}

Done.


pkg/storage/allocator.go, line 325 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…
if badStoreID != 0 {
...
}

done, that's much cleaner.


Comments from Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


pkg/storage/allocator.go, line 407 at r4 (raw file):

		existingCandidates := removeRuleSolver.Solve(existingStoreList, constraints, nil, nil)
		candidates := allocateRuleSolver.Solve(candidateStoreList, constraints, nil, nil)

Don't you need candidates = candidates.onlyValid(). This is exactly why I think there should be separate calls for removeCandidates and allocateCandidates rather than trying to shoehorn both into the same Solve interface. Even if under the hood {remove,alloc}Candidates use Solve, I think it is worthwhile to introduce those methods.


Comments from Reviewable

@BramGruneir BramGruneir force-pushed the rules4 branch 2 times, most recently from 9c48f83 to af7cfda Compare December 1, 2016 18:12
This commit splits the scores returned in the rule solver into a
constraint and a balance score.
Solve now returns all candidates instead of just the valid ones. To
get only the valid candidates, the new function onlyValid and new type
alias condidateList have also been added.

This change allows us to use solve for removeTarget. It also cleans
up the logic in removeTarget to more closely match the non-rule solver
version.
@BramGruneir
Copy link
Member Author

replaced by #12165

@BramGruneir BramGruneir closed this Dec 8, 2016
@BramGruneir BramGruneir deleted the rules4 branch December 19, 2016 16:35
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.

2 participants