-
Notifications
You must be signed in to change notification settings - Fork 455
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
[cluster] Fix unbalanced initial shard allocation for sharded placement #4020
Changes from 2 commits
9c828bc
144fa72
1721594
ebc054b
29e0bf3
49585ee
867979c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1306,6 +1306,42 @@ func TestBalanceShardsForShardedWhenImbalanced(t *testing.T) { | |
assert.Equal(t, expectedInstances, balancedPlacement.Instances()) | ||
} | ||
|
||
func TestInitialPlacementIsBalanced(t *testing.T) { | ||
instances := make([]placement.Instance, 0) | ||
|
||
for i := 0; i < 10; i++ { | ||
instances = append(instances, | ||
placement.NewEmptyInstance(fmt.Sprintf("instance-0-%03d", i), "iso0", "zone", "endpoint", 1), | ||
placement.NewEmptyInstance(fmt.Sprintf("instance-1-%03d", i), "iso1", "zone", "endpoint", 1), | ||
) | ||
} | ||
|
||
ids := make([]uint32, 1024) | ||
for i := range ids { | ||
ids[i] = uint32(i) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From this code I can't get what those ids are. Are they shard ids? Instance ids? Some other ids? Perhaps some naming would help here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
algo := newShardedAlgorithm(placement.NewOptions()) | ||
p, err := algo.InitialPlacement(instances, ids, 2) | ||
require.NoError(t, err) | ||
|
||
var ( | ||
min = math.MaxInt32 | ||
max = math.MinInt32 | ||
) | ||
for _, instance := range p.Instances() { | ||
n := instance.Shards().NumShards() | ||
if n < min { | ||
min = n | ||
} | ||
if n > max { | ||
max = n | ||
} | ||
} | ||
require.True(t, max-min <= 1, | ||
"The number of shards per instance differs by more than 1, min=%d max=%d", min, max) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this is a unit test for a single concrete case, I would simplify it by asserting that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
func verifyAllShardsInAvailableState(t *testing.T, p placement.Placement) { | ||
for _, instance := range p.Instances() { | ||
s := instance.Shards() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be useful to apply property based testing in this case? https://pkg.go.dev/testing/quick
To be more confident we generate well balanced shards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a matter of fact we are using
github.com/leanovate/gopter
.