[cluster] Fix unbalanced initial shard allocation for sharded placement #4020
Conversation
| @@ -1306,6 +1306,42 @@ func TestBalanceShardsForShardedWhenImbalanced(t *testing.T) { | |||
| assert.Equal(t, expectedInstances, balancedPlacement.Instances()) | |||
| } | |||
|
|
|||
| func TestInitialPlacementIsBalanced(t *testing.T) { | |||
There was a problem hiding this comment.
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.
As a matter of fact we are using github.com/leanovate/gopter.
| @@ -1306,6 +1306,42 @@ func TestBalanceShardsForShardedWhenImbalanced(t *testing.T) { | |||
| assert.Equal(t, expectedInstances, balancedPlacement.Instances()) | |||
| } | |||
|
|
|||
| func TestInitialPlacementIsBalanced(t *testing.T) { | |||
There was a problem hiding this comment.
As a matter of fact we are using github.com/leanovate/gopter.
| ids := make([]uint32, 1024) | ||
| for i := range ids { | ||
| ids[i] = uint32(i) | ||
| } |
There was a problem hiding this comment.
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.
| 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.
As this is a unit test for a single concrete case, I would simplify it by asserting that n is within certain range.
What is done here would be more appropriate for a property based test that @Antanukas has suggested (if you decide to go with that approach).
There was a problem hiding this comment.
There was a problem hiding this comment.
Two questions:
1. Is this the code that also generates the placements for m3aggregator?
The same handler is used for initializing m3db, m3coordinator and m3aggregator placements
m3/src/cluster/placementhandler/common.go
Lines 252 to 265 in 82aa22e
There are non_sharded, sharded and mirrored algorithms (I suppose different algorithms are used for dbnodes and aggregators). non_sharded is straigthforward, and mirrored depends on sharded.
2. Is this code used during initial placement generation only, or also when moving shards around during cluster resizing?
Yes. placeShards(), which uses the heap, also is used for other operations.
| ids := make([]uint32, 1024) | ||
| for i := range ids { | ||
| ids[i] = uint32(i) | ||
| } |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
LGTM but I agree with @Antanukas that a property based test feels like a good fit here, as we have a good property to assert on (max - min <= 1), also there are other clear properties to check (such as whether every shard has been included and there are no duplicates).
| require.Equal(t, 102, min) | ||
| require.Equal(t, 103, max) |
There was a problem hiding this comment.
Nit: just do the range assertions in the loop and drop 10 LoC.
There was a problem hiding this comment.
Migrated to property test instead: ebc054b
| testInitialPlacementIsBalanced, | ||
| gen.IntRange(1, 3), | ||
| gen.IntRange(1, 20), | ||
| gen.IntRange(128, 1024), |
There was a problem hiding this comment.
Let's cover a broader shard range based on what we've seen (maybe 64 to 3072).
What this PR does / why we need it:
Fixes unbalanced shard assignment to instances when initializing sharded placement. This was caused by nondeterministic comparison function used in a heap.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: