ES-10037 Persist recent write load in index metadata #125330

PeteGillinElastic · 2025-03-20T16:58:44Z

This changes the default value for the Exponentially Weighted Moving Rate calculation used for the 'recent write load' metric in indexing stats to 5 minutes (as agreed over Slack) and persists the value in the index metadata alongside the existing write load metric.

The value is still not used in the data stream autosharding calculation, that will be yet one more PR.

There are a couple of drive-by changes in this PR:

It adds a comment to DataStreamAutoShardingService.computeOptimalNumberOfShards, because the nested min and max calls are quite hard to understand at a glance.
It changes IndexShard.indexingStats() so that, if it is called before the shard has entered the started state, it uses a timeSinceShardStartedInNanos value of zero when calling InternalIndexingStats.stats(). Previously, it would have passed the current relative time in nanos as timeSinceShardStartedInNanos (because startedRelativeTimeInNanos would be zero) which is arbitrary and incorrect (since the zero point of System.nanoTime() is arbitrary). This didn't actually matter, since InternalIndexingStats.postIndex would not increment the metrics while in recovery, so the numerator used to calculate the write load would be zero if the shard has not started, so it doesn't matter if the denominator is incorrect. However, it is good defensive coding not to rely on that, and to pass a correct value instead.

…r bounds)

PeteGillinElastic · 2025-03-20T17:11:37Z

server/src/test/java/org/elasticsearch/cluster/metadata/IndexWriteLoadTests.java

-                assertThat(indexWriteLoad.getWriteLoadForShard(shardId).getAsDouble(), is(equalTo(populatedShardWriteLoads[shardId])));
-                assertThat(indexWriteLoad.getUptimeInMillisForShard(shardId).isPresent(), is(equalTo(true)));
-                assertThat(indexWriteLoad.getUptimeInMillisForShard(shardId).getAsLong(), is(equalTo(populatedShardUptimes[shardId])));
+                assertThat(indexWriteLoad.getWriteLoadForShard(shardId), equalTo(OptionalDouble.of(populatedShardWriteLoads[shardId])));


I'm combining the separate assertions on isPresent() and getAsDouble/Long() for simplicity.

PeteGillinElastic · 2025-03-20T17:13:58Z

server/src/test/java/org/elasticsearch/cluster/metadata/IndexWriteLoadTests.java

            } else {
-                assertThat(indexWriteLoad.getWriteLoadForShard(shardId).isPresent(), is(false));
-                assertThat(indexWriteLoad.getUptimeInMillisForShard(shardId).isPresent(), is(false));
+                assertThat(indexWriteLoad.getWriteLoadForShard(shardId), equalTo(OptionalDouble.empty()));


It's a minor thing, but here I'm asserting equality with the empty value rather than asserting that isPresent() is false, so that if it fails then the message will say what value it had, which might be helpful.

elasticsearchmachine · 2025-03-20T17:15:18Z

Pinging @elastic/es-data-management (Team:Data Management)

…literals

dakrone

LGTM, I left a couple of really minor comments, but nothing major.

dakrone · 2025-03-20T19:27:45Z

...in/java/org/elasticsearch/action/datastreams/autosharding/DataStreamAutoShardingService.java

+         *  - shardsByMaxThreads = number of shards required to ensure no more than 50% utilization with max number of threads per shard
+         *  - shardsByMaxThreads = number of shards required to ensure no more than 50% utilization with min number of threads per shard


These both say "max" in the name, but I assume one of them should be shardsByMinThreads?

Whoops, good catch. Me and my cut-and-paste again...

dakrone · 2025-03-20T19:31:38Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexWriteLoad.java

@@ -59,35 +68,63 @@ public static IndexWriteLoad create(List<Double> shardsWriteLoad, List<Long> sha
            throw new IllegalArgumentException("At least one shard write load and uptime should be provided, but none was provided");
        }

+        if (shardsRecentWriteLoad != null && shardsRecentWriteLoad.size() != shardsUptimeInMillis.size()) {
+            assert false;


Can you add a message to this failing assert? It helps narrow down the logic if someone encounters it killing a test ES.

It's a fair cop. I lazily cut-and-pasted. I've added messages to all 5 asserts that were missing them in this class.

dakrone · 2025-03-20T19:32:17Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexWriteLoad.java

        assert shardWriteLoad.length == shardUptimeInMillis.length;
        this.shardWriteLoad = shardWriteLoad;
        this.shardUptimeInMillis = shardUptimeInMillis;
+        if (shardRecentWriteLoad != null) {
+            assert shardRecentWriteLoad.length == shardUptimeInMillis.length;


Same here about an assert message

dakrone · 2025-03-20T19:44:21Z

server/src/test/java/org/elasticsearch/index/shard/IndexingStatsSettingsTests.java

+        int tooManyDays = BigDecimal.valueOf(Long.MAX_VALUE)
+            .add(BigDecimal.ONE)
+            .divide(BigDecimal.valueOf(24 * 60 * 60 * 1_000_000_000L), RoundingMode.UP)
+            .setScale(0, RoundingMode.UP)
+            .intValueExact();


Is there a reason this is more complicated here rather than just hard-coding it to a large value for the test? The tests for the limits seem to straddle testing the Settings framework itself, and I'm not sure we're getting a lot of value out of them?

I guess I thought it would specifically assert that exceeding Long.MAX_VALUE nanos would be rejected. But you're right, there's not really much value in these tests. I'll revert.

PeteGillinElastic

Thanks Lee, all comments done.

PeteGillinElastic · 2025-03-20T20:45:29Z

server/src/test/java/org/elasticsearch/index/shard/IndexingStatsSettingsTests.java

+        int tooManyDays = BigDecimal.valueOf(Long.MAX_VALUE)
+            .add(BigDecimal.ONE)
+            .divide(BigDecimal.valueOf(24 * 60 * 60 * 1_000_000_000L), RoundingMode.UP)
+            .setScale(0, RoundingMode.UP)
+            .intValueExact();


I guess I thought it would specifically assert that exceeding Long.MAX_VALUE nanos would be rejected. But you're right, there's not really much value in these tests. I'll revert.

PeteGillinElastic · 2025-03-20T20:51:10Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexWriteLoad.java

@@ -59,35 +68,63 @@ public static IndexWriteLoad create(List<Double> shardsWriteLoad, List<Long> sha
            throw new IllegalArgumentException("At least one shard write load and uptime should be provided, but none was provided");
        }

+        if (shardsRecentWriteLoad != null && shardsRecentWriteLoad.size() != shardsUptimeInMillis.size()) {
+            assert false;


It's a fair cop. I lazily cut-and-pasted. I've added messages to all 5 asserts that were missing them in this class.

PeteGillinElastic · 2025-03-20T20:51:31Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexWriteLoad.java

        assert shardWriteLoad.length == shardUptimeInMillis.length;
        this.shardWriteLoad = shardWriteLoad;
        this.shardUptimeInMillis = shardUptimeInMillis;
+        if (shardRecentWriteLoad != null) {
+            assert shardRecentWriteLoad.length == shardUptimeInMillis.length;


PeteGillinElastic · 2025-03-20T20:52:36Z

...in/java/org/elasticsearch/action/datastreams/autosharding/DataStreamAutoShardingService.java

+         *  - shardsByMaxThreads = number of shards required to ensure no more than 50% utilization with max number of threads per shard
+         *  - shardsByMaxThreads = number of shards required to ensure no more than 50% utilization with min number of threads per shard


Whoops, good catch. Me and my cut-and-paste again...

…cent-write-load

This changes the default value for the Exponentially Weighted Moving Rate calculation used for the 'recent write load' metric in indexing stats to 5 minutes (as agreed over Slack) and persists the value in the index metadata alongside the existing write load metric. The value is still not used in the data stream autosharding calculation, that will be yet one more PR. There are a couple of drive-by changes in this PR: It adds a comment to DataStreamAutoShardingService.computeOptimalNumberOfShards, because the nested min and max calls are quite hard to understand at a glance. It changes IndexShard.indexingStats() so that, if it is called before the shard has entered the started state, it uses a timeSinceShardStartedInNanos value of zero when calling InternalIndexingStats.stats(). Previously, it would have passed the current relative time in nanos as timeSinceShardStartedInNanos (because startedRelativeTimeInNanos would be zero) which is arbitrary and incorrect (since the zero point of System.nanoTime() is arbitrary). This didn't actually matter, since InternalIndexingStats.postIndex would not increment the metrics while in recovery, so the numerator used to calculate the write load would be zero if the shard has not started, so it doesn't matter if the denominator is incorrect. However, it is good defensive coding not to rely on that, and to pass a correct value instead.

elasticsearchmachine added the v9.1.0 label Mar 20, 2025

PeteGillinElastic added >non-issue :Data Management/Stats Statistics tracking and retrieval APIs v9.1.0 and removed v9.1.0 labels Mar 20, 2025

PeteGillinElastic force-pushed the ES-10037-persist-recent-write-load branch 2 times, most recently from 6891b71 to a4465e2 Compare March 20, 2025 17:12

PeteGillinElastic added 4 commits March 20, 2025 17:14

Add a comment to computeOptimalNumberOfShards

6995190

Defensively guard against uninitialized startedRelativeTimeInNanos

15db2c8

Change default half-life to 5mins (also set reasonably upper and lowe…

1391c72

…r bounds)

Persist recent write load in index metadata

6392468

PeteGillinElastic force-pushed the ES-10037-persist-recent-write-load branch from a4465e2 to 6392468 Compare March 20, 2025 17:14

PeteGillinElastic marked this pull request as ready for review March 20, 2025 17:14

PeteGillinElastic commented Mar 20, 2025

View reviewed changes

elasticsearchmachine added the Team:Data Management Meta label for data/management team label Mar 20, 2025

Fix ClusterStateTests which include expected cluster state as string …

bfd85a7

…literals

dakrone approved these changes Mar 20, 2025

View reviewed changes

Respond to review comments

025b7fd

PeteGillinElastic commented Mar 20, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into ES-10037-persist-re…

1f1c954

…cent-write-load

PeteGillinElastic merged commit 22d8169 into elastic:main Mar 20, 2025
17 checks passed

PeteGillinElastic deleted the ES-10037-persist-recent-write-load branch March 20, 2025 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ES-10037 Persist recent write load in index metadata #125330

ES-10037 Persist recent write load in index metadata #125330

PeteGillinElastic commented Mar 20, 2025

PeteGillinElastic Mar 20, 2025

PeteGillinElastic Mar 20, 2025

elasticsearchmachine commented Mar 20, 2025

dakrone left a comment

dakrone Mar 20, 2025

PeteGillinElastic Mar 20, 2025

dakrone Mar 20, 2025

PeteGillinElastic Mar 20, 2025

dakrone Mar 20, 2025

PeteGillinElastic Mar 20, 2025

dakrone Mar 20, 2025

PeteGillinElastic Mar 20, 2025

PeteGillinElastic left a comment

PeteGillinElastic Mar 20, 2025

PeteGillinElastic Mar 20, 2025

PeteGillinElastic Mar 20, 2025

PeteGillinElastic Mar 20, 2025

		* - shardsByMaxThreads = number of shards required to ensure no more than 50% utilization with max number of threads per shard
		* - shardsByMaxThreads = number of shards required to ensure no more than 50% utilization with min number of threads per shard

ES-10037 Persist recent write load in index metadata #125330

ES-10037 Persist recent write load in index metadata #125330

Conversation

PeteGillinElastic commented Mar 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Mar 20, 2025

dakrone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeteGillinElastic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment