Add cache support in `TransportGetAllocationStatsAction` by JeremyDahlgren · Pull Request #124898 · elastic/elasticsearch

JeremyDahlgren · 2025-03-14T16:22:05Z

Adds a new dynamic setting
TransportGetAllocationStatsAction.CACHE_TTL_SETTING "cluster.routing.allocation.stats.cache.ttl" to configure the time to live for cached NodeAllocationStats on the master. The default value is currently 1 minute per the suggestion in issue #110716.

Closes #110716

elasticsearchmachine · 2025-03-14T16:24:10Z

Hi @JeremyDahlgren, I've created a changelog YAML for you.

...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java

DaveCTurner

Great stuff. I've left some suggestions.

...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java

...rg/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsActionTests.java

...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java

elasticsearchmachine · 2025-03-18T19:23:31Z

Hi @JeremyDahlgren, I've created a changelog YAML for you.

JeremyDahlgren · 2025-03-18T19:39:38Z

...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java

+    public static final Setting<TimeValue> CACHE_TTL_SETTING = Setting.timeSetting(
+        "cluster.routing.allocation.stats.cache.ttl",


What visibility/exposure should this setting have? I saw other cluster.routing.allocation.* settings in docs in cluster-level-shard-allocation-routing-settings.md. Should it be documented there?

Yes, we need to add documentation, either in this PR or in follow up. Both are fine.

++ would prefer to add the docs in the same PR, otherwise it tends to get forgotten

elasticsearchmachine · 2025-03-18T19:41:01Z

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

elasticsearchmachine · 2025-03-18T19:41:02Z

Hi @JeremyDahlgren, I've created a changelog YAML for you.

mhl-b

LGTM. Docs update can be a follow up

mhl-b · 2025-03-19T17:23:45Z

...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java

+    public static final Setting<TimeValue> CACHE_TTL_SETTING = Setting.timeSetting(
+        "cluster.routing.allocation.stats.cache.ttl",


Yes, we need to add documentation, either in this PR or in follow up. Both are fine.

mhl-b · 2025-03-19T17:27:40Z

...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java

+        Map<String, NodeAllocationStats> get() {
+
+            if (ttlMillis == 0L) {
+                return null;
+            }
+
+            final var stats = cachedStats.get();
+
+            if (stats == null || threadPool.relativeTimeInMillis() - stats.timestampMillis > ttlMillis) {
+                return null;
+            }
+
+            return stats.stats;
+        }


nit: maybe remove empty lines

...rg/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsActionTests.java

DaveCTurner

Tiny comments only, otherwise LGTM2

DaveCTurner · 2025-03-20T11:09:32Z

...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java

+    public static final Setting<TimeValue> CACHE_TTL_SETTING = Setting.timeSetting(
+        "cluster.routing.allocation.stats.cache.ttl",


++ would prefer to add the docs in the same PR, otherwise it tends to get forgotten

DaveCTurner · 2025-03-20T11:12:22Z

...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java

+            final var stats = cachedStats.get();
+
+            if (stats == null || threadPool.relativeTimeInMillis() - stats.timestampMillis > ttlMillis) {
+                return null;


Should we clear cachedStats here if it's expired? I guess we're just about to overwrite it with a fresh copy so doesn't really matter? If so, could we have a comment just noting this decision for the next person?

...rg/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsActionTests.java

Adds a new setting TransportGetAllocationStatsAction.CACHE_MAX_AGE_SETTING to configure the max age for cached AllocationStats on the master. The default value is currently 1 minute per the suggestion in issue 110716. Closes elastic#110716

…read

mhl-b

Documentation update LGTM too

elasticsearchmachine · 2025-03-21T17:29:06Z

Hi @JeremyDahlgren, I've created a changelog YAML for you.

elasticsearchmachine · 2025-03-21T18:36:42Z

💔 Backport failed

The backport operation could not be completed due to the following error:

There are no branches to backport to. Aborting.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 124898

Adds a new cache and setting TransportGetAllocationStatsAction.CACHE_TTL_SETTING "cluster.routing.allocation.stats.cache.ttl" to configure the max age for cached NodeAllocationStats on the master. The default value is currently 1 minute per the suggestion in issue 110716. Closes elastic#110716

��5588) Adds a new cache and setting TransportGetAllocationStatsAction.CACHE_TTL_SETTING "cluster.routing.allocation.stats.cache.ttl" to configure the max age for cached NodeAllocationStats on the master. The default value is currently 1 minute per the suggestion in issue 110716. Closes #110716

Adds a new cache and setting TransportGetAllocationStatsAction.CACHE_TTL_SETTING "cluster.routing.allocation.stats.cache.ttl" to configure the max age for cached NodeAllocationStats on the master. The default value is currently 1 minute per the suggestion in issue 110716. Closes elastic#110716

JeremyDahlgren added >enhancement :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) Team:Distributed Coordination (obsolete) Meta label for Distributed Coordination team. Obsolete. Please do not use. labels Mar 14, 2025

JeremyDahlgren requested review from DaveCTurner and mhl-b March 14, 2025 16:22

elasticsearchmachine added the v9.1.0 label Mar 14, 2025

JeremyDahlgren commented Mar 14, 2025

View reviewed changes

...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java Outdated Show resolved Hide resolved

...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java Outdated Show resolved Hide resolved

DaveCTurner reviewed Mar 14, 2025

View reviewed changes

JeremyDahlgren mentioned this pull request Mar 14, 2025

Fix test in TransportGetAllocationStatsActionTests #124902

Merged

JeremyDahlgren force-pushed the fix/110716 branch from 6403432 to 53f307c Compare March 14, 2025 21:22

mhl-b reviewed Mar 14, 2025

View reviewed changes

...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java Outdated Show resolved Hide resolved

JeremyDahlgren force-pushed the fix/110716 branch from 84ec448 to 9b87c4b Compare March 18, 2025 19:19

JeremyDahlgren force-pushed the fix/110716 branch from 2c040be to 94be786 Compare March 18, 2025 19:31

JeremyDahlgren commented Mar 18, 2025

View reviewed changes

JeremyDahlgren marked this pull request as ready for review March 18, 2025 19:40

JeremyDahlgren requested a review from a team as a code owner March 18, 2025 19:40

mhl-b approved these changes Mar 19, 2025

View reviewed changes

DaveCTurner reviewed Mar 20, 2025

View reviewed changes

JeremyDahlgren added 8 commits March 20, 2025 16:33

Switch to controlled time in unit test, address code review comments

8db340e

Refactor lambda passed to deduplicator to check cache in transport th…

dd05487

…read

Use timestampMillis name in CachedAllocationStats record

81e32c1

Make the cache TTL a dynamic setting

5d71e1f

Fix testReturnsOnlyRequestedStats after merging 124902

ae438b9

Address code review comments

341cd9c

Add documentation for cluster.routing.allocation.stats.cache.ttl

2e6e6ad

JeremyDahlgren force-pushed the fix/110716 branch from d41aa7f to 2e6e6ad Compare March 20, 2025 20:34

mhl-b approved these changes Mar 21, 2025

View reviewed changes

JeremyDahlgren added the auto-backport Automatically create backport pull requests when merged label Mar 21, 2025

Update docs/changelog/124898.yaml

4555ca2

JeremyDahlgren enabled auto-merge (squash) March 21, 2025 17:30

JeremyDahlgren merged commit d799597 into elastic:main Mar 21, 2025
16 of 17 checks passed

elasticsearchmachine added the backport pending label Mar 21, 2025

shainaraskas added the docs-missing-applies-tags label Jul 22, 2025

shainaraskas mentioned this pull request Jul 22, 2025

Add availability information for node allocation stats cache #131699

Merged

		public static final Setting<TimeValue> CACHE_TTL_SETTING = Setting.timeSetting(
		"cluster.routing.allocation.stats.cache.ttl",

Conversation

JeremyDahlgren commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Mar 14, 2025

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 18, 2025

JeremyDahlgren Mar 18, 2025

Choose a reason for hiding this comment

mhl-b Mar 19, 2025

Choose a reason for hiding this comment

DaveCTurner Mar 20, 2025

Choose a reason for hiding this comment

elasticsearchmachine commented Mar 18, 2025

elasticsearchmachine commented Mar 18, 2025

mhl-b left a comment

Choose a reason for hiding this comment

mhl-b Mar 19, 2025

Choose a reason for hiding this comment

mhl-b Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner Mar 20, 2025

Choose a reason for hiding this comment

DaveCTurner Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhl-b left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Mar 21, 2025

Uh oh!

elasticsearchmachine commented Mar 21, 2025

💔 Backport failed

Labels

5 participants

JeremyDahlgren commented Mar 14, 2025 •

edited

Loading