Add unthrottled path for replicas in ThrottlingAllocationDecider #138545
Conversation
elasticsearchmachine
added
v9.3.0
needs:triage
DiannaHohensee
force-pushed
the
2025/11/21/throttle-decider-unthrottles-replica
branch
from
aef7911 to
bd3d442
Compare
DiannaHohensee
force-pushed
the
2025/11/21/throttle-decider-unthrottles-replica
branch
from
bd3d442 to
4ade9fa
Compare
DiannaHohensee
force-pushed
the
2025/11/21/throttle-decider-unthrottles-replica
branch
from
4ade9fa to
4b5ef05
Compare
DiannaHohensee
added
:Distributed Coordination/Allocation
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| // During simulation, this supports early publishing DesiredBalance, with all unassigned shards assigned. | ||
| // Notably, this bypass is only in simulation decisions. Reconciliation will continue to obey throttling, in particular the | ||
| // requirement to assign a primary before allowing its replicas to begin initializing. | ||
| return allocation.decision(Decision.YES, NAME, "replica allocation is not throttled when simulating"); |
There was a problem hiding this comment.
Is this change possibly redundant since we implemented #134786. If I understand correctly the ThrottlingAllocationDecider won't ever kick in now that we do a single move per balancing round?
I thought that ES-12942 was more similar to this change #115511
Never mind I see now we'll need this with the subsequent changes
| ShardRouting shardRouting1Primary, | ||
| ShardRouting shardRouting1Replica, | ||
| ShardRouting shardRouting2Primary, | ||
| ShardRouting shardRouting2Replica |
There was a problem hiding this comment.
should these have unassigned in the name (e.g. unassignedShard1Primary) or similar?
There was a problem hiding this comment.
Yep, that'd be clearer, done 👍
| String[] indices, | ||
| int numberOfShards, | ||
| List<ShardRouting.Role> replicaRoles | ||
| ) { |
There was a problem hiding this comment.
Should we validate that numberOfShards == replicaRoles.size() ?
There was a problem hiding this comment.
I don't think that has to be true, AFAICT. The number of replicaRoles is equal to the number of replicas, which is not limited to the number of shards.
| } else if (i == numberOfDataNodes) { | ||
| discoBuilder.masterNodeId(node.getId()); | ||
| } | ||
| } |
There was a problem hiding this comment.
If I'm reading it right, the actual number of data nodes ends up being numberOfDataNodes + 1, this seems counter-intuitive, could we change numberOfDataNodes to have the actual number of data nodes?
There was a problem hiding this comment.
Yeah, this is pretty convoluted. I never have found a reason for it in the other helpers. Perhaps some historical testing need that no longer exists.
Fixing 👍
| ShardRouting shardRouting1Primary = TestShardRouting.newShardRouting(testShardId1, null, null, true, ShardRoutingState.UNASSIGNED); | ||
| ShardRouting shardRouting2Primary = TestShardRouting.newShardRouting(testShardId2, null, null, true, ShardRoutingState.UNASSIGNED); | ||
| ShardRouting shardRouting1Replica = TestShardRouting.newShardRouting(testShardId1, null, null, false, ShardRoutingState.UNASSIGNED); | ||
| ShardRouting shardRouting2Replica = TestShardRouting.newShardRouting(testShardId2, null, null, false, ShardRoutingState.UNASSIGNED); |
There was a problem hiding this comment.
Maybe it's not important, but I think we should be able to pull these out of the routing table with
RoutingTable routingTable = clusterState.routingTable(ProjectId.DEFAULT);
routingTable.shardRoutingTable(shardId).primaryShard();
routingTable.shardRoutingTable(shardId).replicaShards().get(0);Then perhaps we can avoid the change to TestShardRouting?
There was a problem hiding this comment.
Yeah, routing table is probably better. I just copy-pasted.
The TestShardRouting change is label tidying, other callers provide null.
nicktindall
left a comment
nicktindall
left a comment
There was a problem hiding this comment.
LGTM with some minor queries
DiannaHohensee
added
the
auto-merge-without-approval
DiannaHohensee
deleted the
2025/11/21/throttle-decider-unthrottles-replica
branch
3 participants
Adds a new cluster setting to allow the ThrottlingAllocationDecider
to bypass replica shard throttling during balancer simulation. Primary
shards already always bypass throttling during simulation so that new
index shards are assigned (and made available) as quickly as possible.
Replicas need the same quick availability in some environments.
Relates ES-12942
I'm splitting the work for ES-12942 into pieces. Next I'll need to make sure the BalancedShardsAllocator#allocate() can assign all the replicas in one call, and then change the DesiredBalanceComputer's early return logic to pick up new assignment of unassigned replicas.