Setting for estimated shard heap allocation decider#128722
Setting for estimated shard heap allocation decider#128722elasticsearchmachine merged 13 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| } | ||
| } else { | ||
| logger.trace("skipping collecting shard heap usage from cluster, notifying listeners with empty shard heap usage"); | ||
| shardHeapUsagePerNode = Map.of(); |
There was a problem hiding this comment.
This complexity looks a bit nasty, but I can't think of a way to make it nicer.
I wonder if it would look better if we made the decisions up front e.g.
var fetchIndicesStats = diskThresholdEnabled;
var fetchNodeStates = diskThresholdEnabled || shardThresholdEnabled;
var fetchShardHeapUsage = shardThresholdEnaled;probably wouldn't make much difference.
What about if we had something like...
enum Requirement {
NodeStats,
IndicesStats,
ShardHeapUsage
}
record Feature(Set<Requirement> requirements) {
}
Feature DISK_THRESHOLD = new Feature(Set.of(NodeStats, IndicesStats));
Feature SHARD_HEAP = new Feature(Set.if(NodeStats, ShardHeapUsage));
// then
// ...
Set<Requirement> requirements = enabledFeatures.stream().flatMap(feat -> feat.requirements().stream()).toSet();
// ...
if (requirements.contains(NodeStats) {
try (var ignored = threadPool.getThreadContext().clearTraceContext()) {
fetchNodeStats();
}
}You could argue its overkill now, but if we keep doing this stuff it'll make things easier to read.
There was a problem hiding this comment.
I am not sure about the 2nd suggestion. At this point, it seems adding to the complexity, e.g. it still needs the three if/else statements at the end. Maybe it can be better hidden if Feature somehow handles the fetches internally. But that seems a bit too much for what we need here. So I pushed d3dd95a to extract each if/else branch into its own method. This is somewhat aligned with your first suggestion. Though it does not reduce the complexity, I feel the readability is better since it seems clearer about when a particular stats need to be fetched. Please let me know if that works for you. Thanks!
There was a problem hiding this comment.
Yeah that improves the readability, thanks.
nicktindall
left a comment
There was a problem hiding this comment.
LGTM with some minor nitpicking
This PR adds a new setting to toggle the collection for shard heap usages as well as wiring ShardHeapUsage into ClusterInfoSimulator.
Relates: #128723