Balancer skips disk related computation when disk weight factor is zero #136352
+121
−13
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,7 @@ | |
| import org.elasticsearch.cluster.routing.allocation.decider.SameShardAllocationDecider; | ||
| import org.elasticsearch.cluster.routing.allocation.decider.ThrottlingAllocationDecider; | ||
| import org.elasticsearch.common.UUIDs; | ||
| import org.elasticsearch.common.collect.Iterators; | ||
| import org.elasticsearch.common.settings.ClusterSettings; | ||
| import org.elasticsearch.common.settings.Settings; | ||
| import org.elasticsearch.common.unit.ByteSizeUnit; | ||
|
|
@@ -63,6 +64,7 @@ | |
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.function.DoubleSupplier; | ||
| import java.util.function.Function; | ||
| import java.util.stream.Collector; | ||
|
|
@@ -74,6 +76,7 @@ | |
| import static java.util.stream.Collectors.summingDouble; | ||
| import static java.util.stream.Collectors.summingLong; | ||
| import static java.util.stream.Collectors.toSet; | ||
| import static org.elasticsearch.cluster.ClusterInfo.shardIdentifierFromRouting; | ||
| import static org.elasticsearch.cluster.routing.ShardRoutingState.RELOCATING; | ||
| import static org.elasticsearch.cluster.routing.TestShardRouting.shardRoutingBuilder; | ||
| import static org.elasticsearch.cluster.routing.allocation.allocator.BalancedShardsAllocator.Balancer.PrioritiseByShardWriteLoadComparator.THRESHOLD_RATIO; | ||
|
|
@@ -614,9 +617,9 @@ public void testShardSizeDiscrepancyWithinIndex() { | |
| () -> ClusterInfo.builder() | ||
| .shardSizes( | ||
| Map.of( | ||
| shardIdentifierFromRouting(new ShardId(index, 0), true), | ||
| 0L, | ||
| shardIdentifierFromRouting(new ShardId(index, 1), true), | ||
| ByteSizeUnit.GB.toBytes(500) | ||
| ) | ||
| ) | ||
|
|
@@ -691,6 +694,87 @@ public void testPartitionedClusterWithSeparateWeights() { | |
| assertThat(shardBalancedPartition.get("shardsOnly-2"), hasSize(3)); | ||
| } | ||
|
|
||
| public void testSkipDiskUsageComputation() { | ||
| final var modelNodesRef = new AtomicReference<BalancedShardsAllocator.ModelNode[]>(); | ||
| final var balancerRef = new AtomicReference<BalancedShardsAllocator.Balancer>(); | ||
| // Intentionally configure disk weight factor to be non-zero so that the test would fail if disk usage is not ignored | ||
| final var weightFunction = new WeightFunction(1, 1, 1, 1); | ||
| final var allocator = new BalancedShardsAllocator( | ||
| BalancerSettings.DEFAULT, | ||
| TEST_WRITE_LOAD_FORECASTER, | ||
| () -> new BalancingWeights() { | ||
| @Override | ||
| public WeightFunction weightFunctionForShard(ShardRouting shard) { | ||
| return weightFunction; | ||
| } | ||
|
|
||
| @Override | ||
| public WeightFunction weightFunctionForNode(RoutingNode node) { | ||
| return weightFunction; | ||
| } | ||
|
|
||
| @Override | ||
| public NodeSorters createNodeSorters( | ||
| BalancedShardsAllocator.ModelNode[] modelNodes, | ||
| BalancedShardsAllocator.Balancer balancer | ||
| ) { | ||
| modelNodesRef.set(modelNodes); | ||
| balancerRef.set(balancer); | ||
| final BalancedShardsAllocator.NodeSorter nodeSorter = new BalancedShardsAllocator.NodeSorter( | ||
| modelNodes, | ||
| weightFunction, | ||
| balancer | ||
| ); | ||
| return new NodeSorters() { | ||
| @Override | ||
| public BalancedShardsAllocator.NodeSorter sorterForShard(ShardRouting shard) { | ||
| return nodeSorter; | ||
| } | ||
|
|
||
| @Override | ||
| public Iterator<BalancedShardsAllocator.NodeSorter> iterator() { | ||
| return Iterators.single(nodeSorter); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean diskUsageIgnored() { | ||
| return true; // This makes the computation ignore disk usage | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| final String indexName = randomIdentifier(); | ||
| final var clusterState = createStateWithIndices(anIndex(indexName).shardSizeInBytesForecast(ByteSizeValue.ofGb(8).getBytes())); | ||
| final var index = clusterState.metadata().getProject(ProjectId.DEFAULT).index(indexName); | ||
|
|
||
| // A cluster info with shard sizes | ||
| final var clusterInfo = new ClusterInfo( | ||
| Map.of(), | ||
| Map.of(), | ||
| Map.of(shardIdentifierFromRouting(new ShardId(index.getIndex(), 0), true), ByteSizeValue.ofGb(8).getBytes()), | ||
| Map.of(), | ||
| Map.of(), | ||
| Map.of(), | ||
| Map.of(), | ||
| Map.of(), | ||
| Map.of(), | ||
| Map.of() | ||
| ); | ||
|
||
| allocator.allocate( | ||
| new RoutingAllocation(new AllocationDeciders(List.of()), clusterState, clusterInfo, null, 0L).mutableCloneForSimulation() | ||
| ); | ||
|
|
||
| final var modelNodes = modelNodesRef.get(); | ||
| assertNotNull(modelNodes); | ||
| final var balancer = balancerRef.get(); | ||
| assertNotNull(balancer); | ||
|
|
||
| assertThat(balancer.avgDiskUsageInBytesPerNode(), equalTo(0.0)); | ||
| Arrays.stream(modelNodes).forEach(modelNode -> assertThat(modelNode.diskUsageInBytes(), equalTo(0.0))); | ||
| } | ||
|
|
||
| public void testReturnEarlyOnShardAssignmentChanges() { | ||
| var shardWriteLoads = new HashMap<ShardId, Double>(); | ||
| var allocationService = new MockAllocationService( | ||
|
|
@@ -1228,6 +1312,11 @@ public BalancedShardsAllocator.NodeSorter sorterForShard(ShardRouting shard) { | |
| } | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean diskUsageIgnored() { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This bit I have minor apprehensions about. But it seems like it's not an easy one to skip on the caller side. And we do have that information available to us here via the balancingWeights.
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.
I can remove this change if you prefer. It does not really show up in the flamegraph. So I could be over-zealous.
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.
Yeah maybe that would be nicer. The behaviour seems a little surprising potentially. It would seem safer if the caller was skipping the call rather than the callee just returning zero.
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.
It turns out that we can check it at the call site. Not sure why I initially thought it was not feasible ... Pushed 0affb99
Let me know if this works for you. Thanks!