Skip to content

Refactor ILMs UpdateSettingsStep to be more generic #133329

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.

Sign up for GitHub

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

Merged
nielsbauman merged 4 commits into from
Aug 22, 2025
Merged

Refactor ILMs UpdateSettingsStep to be more generic #133329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0978063
Refactor ILMs `UpdateSettingsStep` to be more generic
nielsbauman Aug 21, 2025
883ce28
Add javadoc to constructors
nielsbauman Aug 22, 2025
3905671
Change signature of `targetIndexNameSupplier`
nielsbauman Aug 22, 2025
23fde79
Merge branch 'main' into ilm-update-settings
nielsbauman Aug 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 32 additions & 25 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,45 @@
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.Objects;
import java.util.function.BiFunction;
import java.util.function.Function;

/**
* Updates the settings for an index.
*/
public class UpdateSettingsStep extends AsyncActionStep {
public static final String NAME = "update-settings";

private final Settings settings;
private static final BiFunction<String, LifecycleExecutionState, String> DEFAULT_TARGET_INDEX_NAME_SUPPLIER = (index, state) -> index;

private final BiFunction<String, LifecycleExecutionState, String> targetIndexNameSupplier;
Copy link
Contributor Author

@nielsbauman nielsbauman Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the signature of the field to match the signature of similar fields in other steps, such as:

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CopyExecutionStateStep.java

Line 38 in cb2e4db

private final BiFunction<String, LifecycleExecutionState, String> targetIndexNameSupplier;

While I would prefer using Function<IndexMetadata, String> which still gives access to the name and state, and is more compact, having only one ShrinkIndexNameSupplier#getShrinkIndexName is more worth it IMO. We can always refactor all usages if one go if we change our mind.

private final Function<IndexMetadata, Settings> settingsSupplier;

/**
* Use this constructor when you want to update the index that ILM runs on with <i>constant</i> 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,
Client client,
BiFunction<String, LifecycleExecutionState, String> targetIndexNameSupplier,
Function<IndexMetadata, Settings> settingsSupplier
) {
super(key, nextStepKey, client);
this.settings = settings;
this.targetIndexNameSupplier = targetIndexNameSupplier;
this.settingsSupplier = settingsSupplier;
}

@Override
Expand All @@ -42,32 +65,16 @@ public void performAction(
ClusterStateObserver observer,
ActionListener<Void> listener
) {
UpdateSettingsRequest updateSettingsRequest = new UpdateSettingsRequest(indexMetadata.getIndex().getName()).masterNodeTimeout(
TimeValue.MAX_VALUE
).settings(settings);
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);
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) {
Copy link
Contributor Author

@nielsbauman nielsbauman Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's not much value in overriding hashCode and equals when the only two fields are functions (which might be lambdas) and thus do not implement those methods.

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<IndexMetadata, Settings> getSettingsSupplier() {
return settingsSupplier;
}
}
8 changes: 5 additions & 3 deletions x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/AllocateActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Copy link
Contributor Author

@nielsbauman nielsbauman Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered adding a getter like so in UpdateSettingsStep:

// visibility for testing
Settings getSettings() {
	return settingsApplier(null);
}

to avoid having to do this in several tests, but I didn't feel like that was a good place for such test-only code, and I couldn't think of a sensible other place, so decided to update the few tests that required updating.

AllocationRoutedStep secondStep = (AllocationRoutedStep) steps.get(1);
assertEquals(expectedSecondStepKey, secondStep.getKey());
assertEquals(nextStepKey, secondStep.getNextStepKey());
Expand All @@ -204,13 +204,15 @@ public void testTotalNumberOfShards() throws Exception {
);
List<Step> 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);
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/ForceMergeActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
);
}
Expand Down
9 changes: 6 additions & 3 deletions x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/MigrateActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,17 +92,20 @@ public void testMigrateActionsConfiguresTierPreference() {
{
List<Step> 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<Step> 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<Step> 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)
);
}
}

Expand Down
10 changes: 5 additions & 5 deletions ...ck/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/SetPriorityActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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)))
);
}

Expand Down
12 changes: 5 additions & 7 deletions ...k/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStepTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -50,7 +48,7 @@ public UpdateSettingsStep copyInstance(UpdateSettingsStep instance) {
instance.getKey(),
instance.getNextStepKey(),
instance.getClientWithoutProject(),
instance.getSettings()
instance.getSettingsSupplier().apply(null)
);
}

Expand All @@ -71,7 +69,7 @@ public void testPerformAction() throws Exception {
UpdateSettingsRequest request = (UpdateSettingsRequest) invocation.getArguments()[0];
@SuppressWarnings("unchecked")
ActionListener<AcknowledgedResponse> listener = (ActionListener<AcknowledgedResponse>) 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;
Expand All @@ -96,7 +94,7 @@ public void testPerformActionFailure() {
UpdateSettingsRequest request = (UpdateSettingsRequest) invocation.getArguments()[0];
@SuppressWarnings("unchecked")
ActionListener<AcknowledgedResponse> listener = (ActionListener<AcknowledgedResponse>) 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;
Expand Down