Skip to content

Commit

Permalink
Merge pull request #23057 from a-robinson/lessreplicas
Browse files Browse the repository at this point in the history 
*: let per-replica constraints specify less than all a zone's replicas
  • Loading branch information
@a-robinson
a-robinson authored Feb 26, 2018
2 parents 3866e55 + b71f822 commit 57ede7e
Show file tree
Hide file tree
Showing 7 changed files with 355 additions and 77 deletions.
9 changes: 5 additions & 4 deletions pkg/config/zone.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,16 +343,17 @@ func (z *ZoneConfig) Validate() error {
}
numConstrainedRepls += int64(constraints.NumReplicas)
for _, constraint := range constraints.Constraints {
// TODO(a-robinson): Relax this constraint to allow prohibited replicas,
// as discussed on #23014.
if constraint.Type != Constraint_REQUIRED && constraints.NumReplicas != z.NumReplicas {
return fmt.Errorf(
"only required constraints (prefixed with a '+') can be applied to a subset of replicas")
}
}
}
// TODO(a-robinson): Relax this constraint, as discussed on #22412.
if numConstrainedRepls != int64(z.NumReplicas) {
return fmt.Errorf(
"the number of replicas specified in constraints (%d) does not equal the number of replicas configured for the zone (%d)",
if numConstrainedRepls > int64(z.NumReplicas) {
return fmt.Errorf("the number of replicas specified in constraints (%d) cannot be greater "+
"than the number of replicas configured for the zone (%d)",
numConstrainedRepls, z.NumReplicas)
}
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/config/zone_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func TestZoneConfigValidate(t *testing.T) {
},
},
},
"the number of replicas specified in constraints .+ does not equal",
"the number of replicas specified in constraints .+ cannot be greater than",
},
{
ZoneConfig{
Expand All @@ -122,7 +122,7 @@ func TestZoneConfigValidate(t *testing.T) {
},
},
},
"the number of replicas specified in constraints .+ does not equal",
"",
},
{
ZoneConfig{
Expand Down
22 changes: 11 additions & 11 deletions pkg/storage/allocator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ func (a *Allocator) ComputeAction(
// we'll up-replicate to, just an indication that such a target exists.
if _, _, err := a.AllocateTarget(
ctx,
zone.Constraints,
zone,
liveReplicas,
rangeInfo,
disableStatsBasedRebalancing,
Expand Down Expand Up @@ -330,15 +330,15 @@ type decisionDetails struct {
// a store.
func (a *Allocator) AllocateTarget(
ctx context.Context,
constraints []config.Constraints,
zone config.ZoneConfig,
existing []roachpb.ReplicaDescriptor,
rangeInfo RangeInfo,
disableStatsBasedRebalancing bool,
) (*roachpb.StoreDescriptor, string, error) {
sl, aliveStoreCount, throttledStoreCount := a.storePool.getStoreList(rangeInfo.Desc.RangeID, storeFilterThrottled)

analyzedConstraints := analyzeConstraints(
ctx, a.storePool.getStoreDescriptor, rangeInfo.Desc.Replicas, constraints)
ctx, a.storePool.getStoreDescriptor, rangeInfo.Desc.Replicas, zone)
options := a.scorerOptions(disableStatsBasedRebalancing)
candidates := allocateCandidates(
sl, analyzedConstraints, existing, rangeInfo, a.storePool.getLocalities(existing), options,
Expand All @@ -364,15 +364,15 @@ func (a *Allocator) AllocateTarget(
return nil, "", errors.Errorf("%d matching stores are currently throttled", throttledStoreCount)
}
return nil, "", &allocatorError{
constraints: constraints,
constraints: zone.Constraints,
aliveStoreCount: aliveStoreCount,
}
}

func (a Allocator) simulateRemoveTarget(
ctx context.Context,
targetStore roachpb.StoreID,
constraints []config.Constraints,
zone config.ZoneConfig,
candidates []roachpb.ReplicaDescriptor,
rangeInfo RangeInfo,
disableStatsBasedRebalancing bool,
Expand All @@ -386,7 +386,7 @@ func (a Allocator) simulateRemoveTarget(
defer func() {
a.storePool.updateLocalStoreAfterRebalance(targetStore, rangeInfo, roachpb.REMOVE_REPLICA)
}()
return a.RemoveTarget(ctx, constraints, candidates, rangeInfo, disableStatsBasedRebalancing)
return a.RemoveTarget(ctx, zone, candidates, rangeInfo, disableStatsBasedRebalancing)
}

// RemoveTarget returns a suitable replica to remove from the provided replica
Expand All @@ -396,7 +396,7 @@ func (a Allocator) simulateRemoveTarget(
// replicas.
func (a Allocator) RemoveTarget(
ctx context.Context,
constraints []config.Constraints,
zone config.ZoneConfig,
candidates []roachpb.ReplicaDescriptor,
rangeInfo RangeInfo,
disableStatsBasedRebalancing bool,
Expand All @@ -413,7 +413,7 @@ func (a Allocator) RemoveTarget(
sl, _, _ := a.storePool.getStoreListFromIDs(existingStoreIDs, roachpb.RangeID(0), storeFilterNone)

analyzedConstraints := analyzeConstraints(
ctx, a.storePool.getStoreDescriptor, rangeInfo.Desc.Replicas, constraints)
ctx, a.storePool.getStoreDescriptor, rangeInfo.Desc.Replicas, zone)
options := a.scorerOptions(disableStatsBasedRebalancing)
rankedCandidates := removeCandidates(
sl,
Expand Down Expand Up @@ -464,7 +464,7 @@ func (a Allocator) RemoveTarget(
// under-utilized store.
func (a Allocator) RebalanceTarget(
ctx context.Context,
constraints []config.Constraints,
zone config.ZoneConfig,
raftStatus *raft.Status,
rangeInfo RangeInfo,
filter storeFilter,
Expand Down Expand Up @@ -501,7 +501,7 @@ func (a Allocator) RebalanceTarget(
}

analyzedConstraints := analyzeConstraints(
ctx, a.storePool.getStoreDescriptor, rangeInfo.Desc.Replicas, constraints)
ctx, a.storePool.getStoreDescriptor, rangeInfo.Desc.Replicas, zone)
options := a.scorerOptions(disableStatsBasedRebalancing)
results := rebalanceCandidates(
ctx,
Expand Down Expand Up @@ -556,7 +556,7 @@ func (a Allocator) RebalanceTarget(
removeReplica, removeDetails, err := a.simulateRemoveTarget(
ctx,
target.store.StoreID,
constraints,
zone,
replicaCandidates,
rangeInfo,
disableStatsBasedRebalancing)
Expand Down
33 changes: 26 additions & 7 deletions pkg/storage/allocator_scorer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -971,6 +971,11 @@ func storeHasConstraint(store roachpb.StoreDescriptor, c config.Constraint) bool

type analyzedConstraints struct {
constraints []config.Constraints
// True if the per-replica constraints don't fully cover all the desired
// replicas in the range (sum(constraints.NumReplicas) < zone.NumReplicas).
// In such cases, we allow replicas that don't match any of the per-replica
// constraints, but never mark them as necessary.
unconstrainedReplicas bool
// For each set of constraints in the above slice, track which StoreIDs
// satisfy them. This field is unused if there are no constraints.
satisfiedBy [][]roachpb.StoreID
Expand All @@ -988,18 +993,20 @@ func analyzeConstraints(
ctx context.Context,
getStoreDescFn func(roachpb.StoreID) (roachpb.StoreDescriptor, bool),
existing []roachpb.ReplicaDescriptor,
constraints []config.Constraints,
zone config.ZoneConfig,
) analyzedConstraints {
result := analyzedConstraints{
constraints: constraints,
constraints: zone.Constraints,
}

if len(constraints) > 0 {
result.satisfiedBy = make([][]roachpb.StoreID, len(constraints))
if len(zone.Constraints) > 0 {
result.satisfiedBy = make([][]roachpb.StoreID, len(zone.Constraints))
result.satisfies = make(map[roachpb.StoreID][]int)
}

for i, subConstraints := range constraints {
var constrainedReplicas int32
for i, subConstraints := range zone.Constraints {
constrainedReplicas += subConstraints.NumReplicas
for _, repl := range existing {
// If for some reason we don't have the store descriptor (which shouldn't
// happen once a node is hooked into gossip), trust that it's valid. This
Expand All @@ -1012,6 +1019,9 @@ func analyzeConstraints(
}
}
}
if constrainedReplicas > 0 && constrainedReplicas < zone.NumReplicas {
result.unconstrainedReplicas = true
}
return result
}

Expand Down Expand Up @@ -1042,6 +1052,10 @@ func allocateConstraintsCheck(
}
}

if analyzed.unconstrainedReplicas {
valid = true
}

return valid, false
}

Expand All @@ -1058,8 +1072,9 @@ func removeConstraintsCheck(
return true, false
}

// The store satisfies none of the constraints.
if len(analyzed.satisfies[store.StoreID]) == 0 {
// The store satisfies none of the constraints, and the zone is not configured
// to desire more replicas than constraints have been specified for.
if len(analyzed.satisfies[store.StoreID]) == 0 && !analyzed.unconstrainedReplicas {
return false, false
}

Expand Down Expand Up @@ -1112,6 +1127,10 @@ func rebalanceFromConstraintsCheck(
}
}

if analyzed.unconstrainedReplicas {
valid = true
}

return valid, false
}

Expand Down
101 changes: 95 additions & 6 deletions pkg/storage/allocator_scorer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,7 @@ func TestAllocateConstraintsCheck(t *testing.T) {
testCases := []struct {
name string
constraints []config.Constraints
zoneNumReplicas int32
existing []roachpb.StoreID
expectedValid map[roachpb.StoreID]bool
expectedNecessary map[roachpb.StoreID]bool
Expand Down Expand Up @@ -741,12 +742,78 @@ func TestAllocateConstraintsCheck(t *testing.T) {
testStoreUSb: true,
},
},
{
name: "multiple required constraints with NumReplicas and sum(NumReplicas) < zone.NumReplicas",
constraints: []config.Constraints{
{
Constraints: []config.Constraint{
{Value: "a", Type: config.Constraint_REQUIRED},
},
NumReplicas: 1,
},
{
Constraints: []config.Constraint{
{Value: "b", Type: config.Constraint_REQUIRED},
},
NumReplicas: 1,
},
},
zoneNumReplicas: 3,
existing: nil,
expectedValid: map[roachpb.StoreID]bool{
testStoreUSa15: true,
testStoreUSa15Dupe: true,
testStoreUSa1: true,
testStoreUSb: true,
testStoreEurope: true,
},
expectedNecessary: map[roachpb.StoreID]bool{
testStoreUSa15: true,
testStoreUSa15Dupe: true,
testStoreUSa1: true,
testStoreUSb: true,
},
},
{
name: "multiple required constraints with sum(NumReplicas) < zone.NumReplicas and not enough existing replicas",
constraints: []config.Constraints{
{
Constraints: []config.Constraint{
{Value: "a", Type: config.Constraint_REQUIRED},
},
NumReplicas: 1,
},
{
Constraints: []config.Constraint{
{Value: "b", Type: config.Constraint_REQUIRED},
},
NumReplicas: 2,
},
},
zoneNumReplicas: 5,
existing: []roachpb.StoreID{testStoreUSa1},
expectedValid: map[roachpb.StoreID]bool{
testStoreUSa15: true,
testStoreUSa15Dupe: true,
testStoreUSa1: true,
testStoreUSb: true,
testStoreEurope: true,
},
expectedNecessary: map[roachpb.StoreID]bool{
testStoreUSa1: true,
testStoreUSb: true,
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
zone := config.ZoneConfig{
Constraints: tc.constraints,
NumReplicas: tc.zoneNumReplicas,
}
analyzed := analyzeConstraints(
context.Background(), getTestStoreDesc, testStoreReplicas(tc.existing), tc.constraints)
context.Background(), getTestStoreDesc, testStoreReplicas(tc.existing), zone)
for _, s := range testStores {
valid, necessary := allocateConstraintsCheck(s, analyzed)
if e, a := tc.expectedValid[s.StoreID], valid; e != a {
Expand All @@ -769,9 +836,10 @@ func TestRemoveConstraintsCheck(t *testing.T) {
valid, necessary bool
}
testCases := []struct {
name string
constraints []config.Constraints
expected map[roachpb.StoreID]expected
name string
constraints []config.Constraints
zoneNumReplicas int32
expected map[roachpb.StoreID]expected
}{
{
name: "prohibited constraint",
Expand Down Expand Up @@ -844,6 +912,24 @@ func TestRemoveConstraintsCheck(t *testing.T) {
testStoreEurope: {false, false},
},
},
{
name: "required constraint with NumReplicas and sum(NumReplicas) < zone.NumReplicas",
constraints: []config.Constraints{
{
Constraints: []config.Constraint{
{Value: "b", Type: config.Constraint_REQUIRED},
},
NumReplicas: 2,
},
},
zoneNumReplicas: 3,
expected: map[roachpb.StoreID]expected{
testStoreUSa15: {true, false},
testStoreEurope: {true, false},
testStoreUSa1: {true, true},
testStoreUSb: {true, true},
},
},
}

for _, tc := range testCases {
Expand All @@ -855,8 +941,11 @@ func TestRemoveConstraintsCheck(t *testing.T) {
StoreID: storeID,
})
}
analyzed := analyzeConstraints(
context.Background(), getTestStoreDesc, existing, tc.constraints)
zone := config.ZoneConfig{
Constraints: tc.constraints,
NumReplicas: tc.zoneNumReplicas,
}
analyzed := analyzeConstraints(context.Background(), getTestStoreDesc, existing, zone)
for storeID, expected := range tc.expected {
valid, necessary := removeConstraintsCheck(testStores[storeID], analyzed)
if e, a := expected.valid, valid; e != a {
Expand Down
Loading

0 comments on commit 57ede7e

Please sign in to comment.