Skip to content

Commit

Permalink
kvserver: fix bug with computing equivalence class
Browse files Browse the repository at this point in the history
Previously, while computing the stores that belong inside the same equivalence
class, we were incorrectly including an existing store in the list of candidate
stores.

For instance, an equivalence class that should look like the following:
```
eqClass:
	existing: [n1, n2]
	candidates: [n3, n4]
```
was instead being computed as the following:
```
eqClass:
	existing: [n1, n2]
	candidates: [n1, n3, n4]
```

This was throwing things off in the logic implemented by #72296

Release note: None
  • Loading branch information
aayushshah15 committed Dec 17, 2021
1 parent 6ca0da1 commit c5f374a
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 67 deletions.
25 changes: 17 additions & 8 deletions pkg/kv/kvserver/allocator_scorer.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ func (o rangeCountScorerOptions) deterministicForTesting() bool {
func (o rangeCountScorerOptions) shouldRebalanceBasedOnThresholds(
ctx context.Context, store roachpb.StoreDescriptor, sl StoreList,
) bool {
if len(sl.stores) == 0 {
return false
}
overfullThreshold := int32(math.Ceil(overfullRangeThreshold(o, sl.candidateRanges.mean)))
// 1. We rebalance if `store` is too far above the mean (i.e. stores
// that are overfull).
Expand Down Expand Up @@ -221,6 +224,9 @@ func (o qpsScorerOptions) deterministicForTesting() bool {
func (o qpsScorerOptions) shouldRebalanceBasedOnThresholds(
ctx context.Context, store roachpb.StoreDescriptor, sl StoreList,
) bool {
if len(sl.stores) == 0 {
return false
}
// 1. We rebalance if `store` is too far above the mean (i.e. stores
// that are overfull).
overfullThreshold := overfullQPSThreshold(o, sl.candidateQueriesPerSecond.mean)
Expand Down Expand Up @@ -866,6 +872,11 @@ func rankedCandidateListForRebalancing(
}
var comparableCands candidateList
for _, store := range allStores.stores {
// Only process replacement candidates, not existing stores.
if store.StoreID == existing.store.StoreID {
continue
}

// Ignore any stores on dead nodes or stores that contain any of the
// replicas within `replicasOnExemptedStores`.
if !isStoreValidForRoutineReplicaTransfer(ctx, store.StoreID) {
Expand Down Expand Up @@ -951,22 +962,20 @@ func rankedCandidateListForRebalancing(
var shouldRebalanceCheck bool
if !needRebalance {
for _, existing := range existingStores {
var sl StoreList
var candidateSL StoreList
outer:
for _, comparable := range equivalenceClasses {
for _, existingCand := range comparable.existing {
if existing.store.StoreID == existingCand.StoreID {
sl = comparable.candidateSL
candidateSL = comparable.candidateSL
break outer
}
}
}
// NB: Due to step 2 from above, we're guaranteed to have a non-empty `sl`
// at this point.
//
// TODO(a-robinson): Some moderate refactoring could extract this logic
// out into the loop below, avoiding duplicate balanceScore calculations.
if options.shouldRebalanceBasedOnThresholds(ctx, existing.store, sl) {
// NB: If we have any candidates that are at least as good as the existing
// replicas in terms of diversity and disk fullness, check whether the
// existing replicas' stats are divergent enough to justify a rebalance.
if options.shouldRebalanceBasedOnThresholds(ctx, existing.store, candidateSL) {
shouldRebalanceCheck = true
break
}
Expand Down
Loading

0 comments on commit c5f374a

Please sign in to comment.