-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Setting for estimated shard heap allocation decider #128722
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
Setting for estimated shard heap allocation decider #128722
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that improves the readability, thanks.
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.
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