Skip to content

Add shard write-load to cluster info #131496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

nicktindall
Copy link
Contributor

@nicktindall nicktindall commented Jul 18, 2025

We already use TransportIndicesStatsAction to retrieve size on disk for the disk threshold decider.

This PR just adds the Indexing stats to that request when the write load decider is enabled, and adds the returned shard write loads to the ClusterInfo.

We should be able to feed these into the simulator to properly account for shard movement.

Relates: ES-12419 & ES-12420

@nicktindall nicktindall added >non-issue :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) labels Jul 18, 2025
@nicktindall nicktindall removed the request for review from DiannaHohensee July 18, 2025 03:30
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

@elasticsearchmachine elasticsearchmachine added Team:Distributed Coordination Meta label for Distributed Coordination team v9.2.0 labels Jul 18, 2025
@@ -527,8 +538,6 @@ public ClusterInfo getClusterInfo() {
estimatedHeapUsages.put(nodeId, new EstimatedHeapUsage(nodeId, maxHeapSize.getBytes(), estimatedHeapUsage));
}
});
final Map<String, NodeUsageStatsForThreadPools> nodeThreadPoolUsageStats = new HashMap<>();
nodeThreadPoolUsageStatsPerNode.forEach((nodeId, nodeWriteLoad) -> { nodeThreadPoolUsageStats.put(nodeId, nodeWriteLoad); });
Copy link
Contributor Author

@nicktindall nicktindall Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appeared to be just a copy, which already happens in the ClusterInfo constructor, I assume remnants of something that was since refactored away.

Copy link
Contributor

@mhl-b mhl-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice

Copy link
Contributor

@DiannaHohensee DiannaHohensee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all my comments are straightforward to address, so I'll go ahead and approve now 👍

@@ -621,6 +621,7 @@ public void testUnassignedAllocationPredictsDiskUsage() {
ImmutableOpenMap.of(),
ImmutableOpenMap.of(),
ImmutableOpenMap.of(),
ImmutableOpenMap.of(),
ImmutableOpenMap.of()
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I missed a handful of these in the builder conversion 😏 Oops..

InternalClusterInfoService.buildShardLevelInfo(stats, shardSizes, shardDataSetSizes, routingToPath, new HashMap<>());
InternalClusterInfoService.buildShardLevelInfo(
stats,
shardWriteLoads,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below there are checks on the results. Are there some checks that can be added for shardWriteLoads results?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in c5ceb92

@@ -59,9 +59,10 @@ public class ClusterInfo implements ChunkedToXContent, Writeable {
final Map<NodeAndPath, ReservedSpace> reservedSpace;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class comment further up ^ could use an update at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cfe5759

@@ -159,7 +159,8 @@ public ClusterInfo getClusterInfo() {
dataPath,
Map.of(),
estimatedHeapUsages,
nodeThreadPoolUsageStats
nodeThreadPoolUsageStats,
allocation.clusterInfo().getShardWriteLoads()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be saved as a private variable initialized in the ClusterInfoSimulator constructor? Similar to estimatedHeapUsages and nodeThreadPoolUsageStats

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we won't use it at this level in the ClusterInfoSimulator and I don't think we'll modify it as part of the simulation either so I think it's fine to pass it through.

public Map<ShardId, Double> getShardWriteLoads() {
return shardWriteLoads;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update the #equals, #hashCode methods below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that we add equals and hashCode just for testing, but I updated them in cd3169f

);

try {
// Force a ClusterInfo refresh to run collection of the node thread pool usage stats.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy-paste failed you here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7886659

}
}
// And that at least one is greater than zero
assertThat(maximumLoadRecorded, greaterThan(0.0));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this assert be per index? Since all the indices received writes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we insert between 1 and 500 documents in indices with between 1 and 3 shards, so it's possible some shards will not be written to. This is just a sanity check anyhow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) >non-issue Team:Distributed Coordination Meta label for Distributed Coordination team v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants