Fetch node and shard heap metrics in a single call - #158087
Conversation
There was a problem hiding this comment.
Pull request overview
Unifies node and shard heap metric collection so both estimates derive from one stateless memory snapshot.
Changes:
- Adds a combined heap-usage result and collector API.
- Snapshots mutable shard metrics before calculating estimates.
- Updates refresh logic, integrations, and tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
server/.../EstimatedHeapUsageCollector.java |
Unifies collector methods. |
server/.../EstimatedHeapUsageStats.java |
Adds combined immutable result. |
server/.../InternalClusterInfoService.java |
Fetches both estimates together. |
server/.../InternalClusterInfoServiceSchedulingTests.java |
Updates scheduling assertions. |
server/.../InternalClusterInfoServiceRefreshTests.java |
Tests combined refresh handling. |
server/.../IndexShardIT.java |
Updates test collector implementation. |
x-pack/.../StatelessHeapUsageReader.java |
Delegates combined collection. |
x-pack/.../StatelessMemoryMetricsService.java |
Calculates from copied shard metrics. |
x-pack/.../StatelessHeapUsageReaderTests.java |
Tests reader delegation and failures. |
Suppressed comments (1)
x-pack/plugin/stateless/src/main/java/org/elasticsearch/xpack/stateless/memory/StatelessMemoryMetricsService.java:690
- This new public method needs Javadoc under the repository's public-method documentation rule. Document that synchronization captures a field-consistent copy for the combined estimate snapshot; adding
@Overridealso makes its relationship toObject.clone()explicit.
public synchronized ShardMemoryMetrics clone() {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * Computes node and shard heap usage estimates from the same snapshot of shard memory metrics. | ||
| */ | ||
| public EstimatedHeapUsageStats getEstimatedHeapUsageStats(ClusterState clusterState) { | ||
| Map<ShardId, ShardMemoryMetrics> shardMemoryMetricsSnapshot = copyShardMemoryMetrics(); |
surya-estc
added
>non-issue
:Distributed/Distributed
|
Pinging @elastic/es-distributed (Team:Distributed) |
nicktindall
left a comment
nicktindall
left a comment
There was a problem hiding this comment.
LGTM, only minor comments
| Map<ShardId, ShardMemoryMetrics> snapshot = new HashMap<>(); | ||
| shardMemoryMetrics.forEach((shardId, shardMemoryMetric) -> snapshot.put(shardId, shardMemoryMetric.snapshot())); | ||
| return snapshot; | ||
| } |
There was a problem hiding this comment.
I'm mildly concerned about this. It's right to take the snapshot so we don't get half-updated metrics for any shards, but I asked Claude and it said it'd be a ~12MB allocation for a 100k shard cluster. I don't think that's crazy large, and I imagine a cluster of that size would be on relatively large nodes, so probably not worth worrying about. I think we'll be able to remove it when we implement elastic/elasticsearch-team#4572 in any case. I checked in serverless and it's unlikely to be an issue for any of the existing large projects.
Uh oh!
There was an error while loading. Please reload this page.