WriteLoadConstraintDecider: Always allow allocation of unassigned shards #135245
Conversation
nicktindall
added
>non-issue
:Distributed Coordination/Allocation
elasticsearchmachine
added
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
nicktindall
requested review from
mhl-b,
DiannaHohensee and
Copilot
and removed request for
mhl-b
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the WriteLoadConstraintDecider to always allow allocation of unassigned shards, regardless of node capacity constraints. The change prevents the decider from blocking initial shard assignments while still enforcing write load constraints for shard relocations.
- Added early return for unassigned shards in the allocation decision logic
- Enhanced test coverage with dedicated unassigned shard test case
- Improved test infrastructure with pattern-based assertion helper
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| WriteLoadConstraintDecider.java | Added check to always allow allocation of unassigned shards |
| WriteLoadConstraintDeciderTests.java | Added test case for unassigned shards and improved test assertion infrastructure |
Comments suppressed due to low confidence (2)
server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderTests.java:1
- The hardcoded values in the error message pattern (0.99, 0.900000) make the test brittle. Consider using variables or extracting these values to constants to improve maintainability and reduce duplication with the test setup.
/*
server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderTests.java:1
- Similar to the previous comment, this error message pattern contains multiple hardcoded values (0.900000, 0.89, 0.06250, 0.50000, 8) that should be extracted to constants or variables to improve test maintainability and reduce coupling with implementation details.
/*
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "Unassigned shard should always be accepted", | ||
| writeLoadDecider.canAllocate( | ||
| testHarness.unassignedShardRouting, | ||
| testHarness.exceedingThresholdRoutingNode, |
There was a problem hiding this comment.
Nit: I guess we could random between exceeding and below threshold routingNodes.
There was a problem hiding this comment.
Fixed in 4527995
| ShardRouting unassignedShardRouting = TestShardRouting.newShardRouting( | ||
| testShardId4Unassigned, | ||
| null, | ||
| true, | ||
| ShardRoutingState.UNASSIGNED | ||
| ); |
There was a problem hiding this comment.
Nit: We can randomly also add write load for this shard into ClusterInfo.
There was a problem hiding this comment.
Yep, fixed in 4527995
# Conflicts: # server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderTests.java
…igned' into ES_11999_always_return_YES_unassigned
Closes: ES-11999