Simulate shards moved by explicit commands #136066
Conversation
In DesiredBalancerComputer, shard movements should be simulated by starting any shards that are initializatng. Previously the explicitly moved shards are not covered. This PR fixes it. Resolves: ES-12943
ywangd
requested review from
nicktindall,
DiannaHohensee,
mhl-b and
henningandersen
ywangd
added
>enhancement
:Distributed Coordination/Allocation
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
|
Hi @ywangd, I've created a changelog YAML for you. |
...main/java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceComputer.java
Outdated
Show resolved
Hide resolved
...java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceComputerTests.java
Show resolved
Hide resolved
...java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceComputerTests.java
Outdated
Show resolved
Hide resolved
...java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceComputerTests.java
Show resolved
Hide resolved
...java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceComputerTests.java
Show resolved
Hide resolved
| final ShardRouting[] initializingShards = routingNodes.node( | ||
| routingAllocation.nodes().resolveNode(command.toNode()).getId() | ||
| ).initializing(); | ||
| assert initializingShards.length == 1 |
There was a problem hiding this comment.
Can we assert no initializing shards after the while loop in general? That helps understand this assertion without reading the prior code.
There was a problem hiding this comment.
This is asserted inside BalancedShardAllocator to ensure each simulation call starts without any initialising shad.
| routingAllocation.nodes().resolveNode(command.toNode()).getId() | ||
| ).initializing(); | ||
| assert initializingShards.length == 1 | ||
| && routingAllocation.nodes() |
There was a problem hiding this comment.
nit: let us split into two assertions, one for only 1 initializing shard and one for the right node holding the initializing shard.
| @Override | ||
| public Decision canAllocate(ShardRouting shardRouting, RoutingNode node, RoutingAllocation allocation) { | ||
| // Move command works every decision except NO | ||
| return randomFrom(Decision.YES, Decision.THROTTLE, Decision.THROTTLE); |
There was a problem hiding this comment.
Did you mean not-preferred rather than 2xthrottle?
There was a problem hiding this comment.
Should have been not-preferred, updated in 5fbd0f9
| var desiredBalanceComputer = createDesiredBalanceComputer(new ShardsAllocator() { | ||
| @Override | ||
| public void allocate(RoutingAllocation allocation) { | ||
| assertThat( |
There was a problem hiding this comment.
Perhaps add a comment that this runs right after the moves are applied.
There was a problem hiding this comment.
Hmm, perhaps this is actually better as an assertion? Which I sort of proposed above. Happy to keep it though for the test to have verification, but we'll probably hit the assertion first.
There was a problem hiding this comment.
Added comment in fcc6449
In BalancedShardsAllocator, we do assert that the initial routingAlloction has no initializing shard. Is it similar to what you have in mind? It's not necessary that allocate runs "right" after the move commands, e.g. we can have additional code after the move commands. The important thing is that all moving shards are started once computer is ready to call allocate.
|
@elasticmachine update branch |
ywangd
added
the
auto-merge-without-approval
In DesiredBalancerComputer, shard movements should be simulated by starting any shards that are initializatng. Previously the explicitly moved shards are not covered. This PR fixes it.
Resolves: ES-12943