From 097806364f2b75dcf99ffd134a7bee59b1f7a2ff Mon Sep 17 00:00:00 2001 From: Niels Bauman Date: Thu, 21 Aug 2025 14:37:32 +0200 Subject: [PATCH 1/3] Refactor ILMs `UpdateSettingsStep` to be more generic By making the step more generic, we can reuse it in other actions (such as the upcoming improvements to the force-merge action). The change is relatively straightforward as the name of the step is already generic enough. --- .../xpack/core/ilm/UpdateSettingsStep.java | 49 +++++++++---------- .../xpack/core/ilm/AllocateActionTests.java | 8 +-- .../xpack/core/ilm/ForceMergeActionTests.java | 2 +- .../xpack/core/ilm/MigrateActionTests.java | 9 ++-- .../core/ilm/SetPriorityActionTests.java | 10 ++-- .../core/ilm/UpdateSettingsStepTests.java | 12 ++--- 6 files changed, 46 insertions(+), 44 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java index cde9c83465cca..daafbf5951764 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java @@ -15,7 +15,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.core.TimeValue; -import java.util.Objects; +import java.util.function.Function; /** * Updates the settings for an index. @@ -23,11 +23,26 @@ public class UpdateSettingsStep extends AsyncActionStep { public static final String NAME = "update-settings"; - private final Settings settings; + private static final Function DEFAULT_TARGET_INDEX_NAME_SUPPLIER = indexMetadata -> indexMetadata.getIndex() + .getName(); + + private final Function targetIndexNameSupplier; + private final Function settingsSupplier; public UpdateSettingsStep(StepKey key, StepKey nextStepKey, Client client, Settings settings) { + this(key, nextStepKey, client, DEFAULT_TARGET_INDEX_NAME_SUPPLIER, indexMetadata -> settings); + } + + public UpdateSettingsStep( + StepKey key, + StepKey nextStepKey, + Client client, + Function targetIndexNameSupplier, + Function settingsSupplier + ) { super(key, nextStepKey, client); - this.settings = settings; + this.targetIndexNameSupplier = targetIndexNameSupplier; + this.settingsSupplier = settingsSupplier; } @Override @@ -42,32 +57,16 @@ public void performAction( ClusterStateObserver observer, ActionListener listener ) { - UpdateSettingsRequest updateSettingsRequest = new UpdateSettingsRequest(indexMetadata.getIndex().getName()).masterNodeTimeout( - TimeValue.MAX_VALUE - ).settings(settings); + String indexName = targetIndexNameSupplier.apply(indexMetadata); + Settings settings = settingsSupplier.apply(indexMetadata); + UpdateSettingsRequest updateSettingsRequest = new UpdateSettingsRequest(indexName).masterNodeTimeout(TimeValue.MAX_VALUE) + .settings(settings); getClient(currentState.projectId()).admin() .indices() .updateSettings(updateSettingsRequest, listener.delegateFailureAndWrap((l, r) -> l.onResponse(null))); } - public Settings getSettings() { - return settings; - } - - @Override - public int hashCode() { - return Objects.hash(super.hashCode(), settings); - } - - @Override - public boolean equals(Object obj) { - if (obj == null) { - return false; - } - if (getClass() != obj.getClass()) { - return false; - } - UpdateSettingsStep other = (UpdateSettingsStep) obj; - return super.equals(obj) && Objects.equals(settings, other.settings); + public Function getSettingsSupplier() { + return settingsSupplier; } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/AllocateActionTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/AllocateActionTests.java index c5a8185f8511b..b55484d14b161 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/AllocateActionTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/AllocateActionTests.java @@ -186,7 +186,7 @@ public void testToSteps() { expectedSettings.put(ShardsLimitAllocationDecider.INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey(), action.getTotalShardsPerNode()); } - assertThat(firstStep.getSettings(), equalTo(expectedSettings.build())); + assertThat(firstStep.getSettingsSupplier().apply(null), equalTo(expectedSettings.build())); AllocationRoutedStep secondStep = (AllocationRoutedStep) steps.get(1); assertEquals(expectedSecondStepKey, secondStep.getKey()); assertEquals(nextStepKey, secondStep.getNextStepKey()); @@ -204,13 +204,15 @@ public void testTotalNumberOfShards() throws Exception { ); List steps = action.toSteps(null, phase, nextStepKey); UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(0); - assertEquals(totalShardsPerNode, firstStep.getSettings().getAsInt(INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey(), null)); + Settings actualSettings = firstStep.getSettingsSupplier().apply(null); + assertEquals(totalShardsPerNode, actualSettings.getAsInt(INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey(), null)); totalShardsPerNode = null; action = new AllocateAction(numberOfReplicas, totalShardsPerNode, null, null, null); steps = action.toSteps(null, phase, nextStepKey); firstStep = (UpdateSettingsStep) steps.get(0); - assertEquals(null, firstStep.getSettings().get(INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey())); + actualSettings = firstStep.getSettingsSupplier().apply(null); + assertNull(actualSettings.get(INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey())); // allow an allocate action that only specifies total shards per node (don't expect any exceptions in this case) action = new AllocateAction(null, 5, null, null, null); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/ForceMergeActionTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/ForceMergeActionTests.java index b8d480200fb5d..9941fb7db5c7d 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/ForceMergeActionTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/ForceMergeActionTests.java @@ -140,7 +140,7 @@ private void assertBestCompression(ForceMergeAction instance) { UpdateSettingsStep updateCodecStep = (UpdateSettingsStep) steps.get(5); assertThat( - updateCodecStep.getSettings().get(EngineConfig.INDEX_CODEC_SETTING.getKey()), + updateCodecStep.getSettingsSupplier().apply(null).get(EngineConfig.INDEX_CODEC_SETTING.getKey()), equalTo(CodecService.BEST_COMPRESSION_CODEC) ); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/MigrateActionTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/MigrateActionTests.java index 4eb6af590494c..de1fbaa86fa1b 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/MigrateActionTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/MigrateActionTests.java @@ -92,17 +92,20 @@ public void testMigrateActionsConfiguresTierPreference() { { List steps = MigrateAction.ENABLED.toSteps(null, HOT_PHASE, nextStepKey); UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(1); - assertThat(DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettings()), is(DATA_HOT)); + assertThat(DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettingsSupplier().apply(null)), is(DATA_HOT)); } { List steps = MigrateAction.ENABLED.toSteps(null, WARM_PHASE, nextStepKey); UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(1); - assertThat(DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettings()), is(DATA_WARM + "," + DATA_HOT)); + assertThat(DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettingsSupplier().apply(null)), is(DATA_WARM + "," + DATA_HOT)); } { List steps = MigrateAction.ENABLED.toSteps(null, COLD_PHASE, nextStepKey); UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(1); - assertThat(DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettings()), is(DATA_COLD + "," + DATA_WARM + "," + DATA_HOT)); + assertThat( + DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettingsSupplier().apply(null)), + is(DATA_COLD + "," + DATA_WARM + "," + DATA_HOT) + ); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/SetPriorityActionTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/SetPriorityActionTests.java index 869af19c34b35..ede84980339d8 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/SetPriorityActionTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/SetPriorityActionTests.java @@ -69,8 +69,8 @@ public void testToSteps() { UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(0); assertThat(firstStep.getKey(), equalTo(expectedFirstStepKey)); assertThat(firstStep.getNextStepKey(), equalTo(nextStepKey)); - assertThat(firstStep.getSettings().size(), equalTo(1)); - assertEquals(priority, (long) IndexMetadata.INDEX_PRIORITY_SETTING.get(firstStep.getSettings())); + assertThat(firstStep.getSettingsSupplier().apply(null).size(), equalTo(1)); + assertEquals(priority, (long) IndexMetadata.INDEX_PRIORITY_SETTING.get(firstStep.getSettingsSupplier().apply(null))); } public void testNullPriorityStep() { @@ -88,10 +88,10 @@ public void testNullPriorityStep() { UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(0); assertThat(firstStep.getKey(), equalTo(expectedFirstStepKey)); assertThat(firstStep.getNextStepKey(), equalTo(nextStepKey)); - assertThat(firstStep.getSettings().size(), equalTo(1)); + assertThat(firstStep.getSettingsSupplier().apply(null).size(), equalTo(1)); assertThat( - IndexMetadata.INDEX_PRIORITY_SETTING.get(firstStep.getSettings()), - equalTo(IndexMetadata.INDEX_PRIORITY_SETTING.getDefault(firstStep.getSettings())) + IndexMetadata.INDEX_PRIORITY_SETTING.get(firstStep.getSettingsSupplier().apply(null)), + equalTo(IndexMetadata.INDEX_PRIORITY_SETTING.getDefault(firstStep.getSettingsSupplier().apply(null))) ); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStepTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStepTests.java index dce5211cad4e8..8fe7c210b8247 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStepTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStepTests.java @@ -32,16 +32,14 @@ public UpdateSettingsStep createRandomInstance() { public UpdateSettingsStep mutateInstance(UpdateSettingsStep instance) { StepKey key = instance.getKey(); StepKey nextKey = instance.getNextStepKey(); - Settings settings = instance.getSettings(); - switch (between(0, 2)) { + switch (between(0, 1)) { case 0 -> key = new StepKey(key.phase(), key.action(), key.name() + randomAlphaOfLength(5)); case 1 -> nextKey = new StepKey(nextKey.phase(), nextKey.action(), nextKey.name() + randomAlphaOfLength(5)); - case 2 -> settings = Settings.builder().put(settings).put(randomAlphaOfLength(10), randomInt()).build(); default -> throw new AssertionError("Illegal randomisation branch"); } - return new UpdateSettingsStep(key, nextKey, client, settings); + return new UpdateSettingsStep(key, nextKey, client, instance.getSettingsSupplier().apply(null)); } @Override @@ -50,7 +48,7 @@ public UpdateSettingsStep copyInstance(UpdateSettingsStep instance) { instance.getKey(), instance.getNextStepKey(), instance.getClientWithoutProject(), - instance.getSettings() + instance.getSettingsSupplier().apply(null) ); } @@ -71,7 +69,7 @@ public void testPerformAction() throws Exception { UpdateSettingsRequest request = (UpdateSettingsRequest) invocation.getArguments()[0]; @SuppressWarnings("unchecked") ActionListener listener = (ActionListener) invocation.getArguments()[1]; - assertThat(request.settings(), equalTo(step.getSettings())); + assertThat(request.settings(), equalTo(step.getSettingsSupplier().apply(indexMetadata))); assertThat(request.indices(), equalTo(new String[] { indexMetadata.getIndex().getName() })); listener.onResponse(AcknowledgedResponse.TRUE); return null; @@ -96,7 +94,7 @@ public void testPerformActionFailure() { UpdateSettingsRequest request = (UpdateSettingsRequest) invocation.getArguments()[0]; @SuppressWarnings("unchecked") ActionListener listener = (ActionListener) invocation.getArguments()[1]; - assertThat(request.settings(), equalTo(step.getSettings())); + assertThat(request.settings(), equalTo(step.getSettingsSupplier().apply(indexMetadata))); assertThat(request.indices(), equalTo(new String[] { indexMetadata.getIndex().getName() })); listener.onFailure(exception); return null; From 883ce2890e6e22e63c3417baf75c103c1f416966 Mon Sep 17 00:00:00 2001 From: Niels Bauman Date: Fri, 22 Aug 2025 07:21:12 +0200 Subject: [PATCH 2/3] Add javadoc to constructors --- .../elasticsearch/xpack/core/ilm/UpdateSettingsStep.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java index daafbf5951764..8c3755ec91a4e 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java @@ -29,10 +29,17 @@ public class UpdateSettingsStep extends AsyncActionStep { private final Function targetIndexNameSupplier; private final Function settingsSupplier; + /** + * Use this constructor when you want to update the index that ILM runs on with constant settings. + */ public UpdateSettingsStep(StepKey key, StepKey nextStepKey, Client client, Settings settings) { this(key, nextStepKey, client, DEFAULT_TARGET_INDEX_NAME_SUPPLIER, indexMetadata -> settings); } + /** + * Use this constructor when you want to update an index other than the one ILM runs on, and/or when you have non-constant settings + * (i.e., settings that depend on the index metadata). + */ public UpdateSettingsStep( StepKey key, StepKey nextStepKey, From 3905671288a85286045abeeaeb36910fa14a36cd Mon Sep 17 00:00:00 2001 From: Niels Bauman Date: Fri, 22 Aug 2025 07:21:42 +0200 Subject: [PATCH 3/3] Change signature of `targetIndexNameSupplier` --- .../xpack/core/ilm/UpdateSettingsStep.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java index 8c3755ec91a4e..57a3157936360 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java @@ -12,9 +12,11 @@ import org.elasticsearch.cluster.ClusterStateObserver; import org.elasticsearch.cluster.ProjectState; import org.elasticsearch.cluster.metadata.IndexMetadata; +import org.elasticsearch.cluster.metadata.LifecycleExecutionState; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.core.TimeValue; +import java.util.function.BiFunction; import java.util.function.Function; /** @@ -23,10 +25,9 @@ public class UpdateSettingsStep extends AsyncActionStep { public static final String NAME = "update-settings"; - private static final Function DEFAULT_TARGET_INDEX_NAME_SUPPLIER = indexMetadata -> indexMetadata.getIndex() - .getName(); + private static final BiFunction DEFAULT_TARGET_INDEX_NAME_SUPPLIER = (index, state) -> index; - private final Function targetIndexNameSupplier; + private final BiFunction targetIndexNameSupplier; private final Function settingsSupplier; /** @@ -44,7 +45,7 @@ public UpdateSettingsStep( StepKey key, StepKey nextStepKey, Client client, - Function targetIndexNameSupplier, + BiFunction targetIndexNameSupplier, Function settingsSupplier ) { super(key, nextStepKey, client); @@ -64,7 +65,7 @@ public void performAction( ClusterStateObserver observer, ActionListener listener ) { - String indexName = targetIndexNameSupplier.apply(indexMetadata); + String indexName = targetIndexNameSupplier.apply(indexMetadata.getIndex().getName(), indexMetadata.getLifecycleExecutionState()); Settings settings = settingsSupplier.apply(indexMetadata); UpdateSettingsRequest updateSettingsRequest = new UpdateSettingsRequest(indexName).masterNodeTimeout(TimeValue.MAX_VALUE) .settings(settings);