Skip to content

Commit b82f28e

Browse files
nielsbaumanpabloem
nielsbauman
authored and
pabloem
committed
Refactor ILMs UpdateSettingsStep to be more generic (elastic#133329)
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.
1 parent b10bf0a commit b82f28e

File tree

6 files changed

+54
-44
lines changed

6 files changed

+54
-44
lines changed

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

Lines changed: 32 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -12,22 +12,45 @@
1212
import org.elasticsearch.cluster.ClusterStateObserver;
1313
import org.elasticsearch.cluster.ProjectState;
1414
import org.elasticsearch.cluster.metadata.IndexMetadata;
15+
import org.elasticsearch.cluster.metadata.LifecycleExecutionState;
1516
import org.elasticsearch.common.settings.Settings;
1617
import org.elasticsearch.core.TimeValue;
1718

18-
import java.util.Objects;
19+
import java.util.function.BiFunction;
20+
import java.util.function.Function;
1921

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

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

30+
private final BiFunction<String, LifecycleExecutionState, String> targetIndexNameSupplier;
31+
private final Function<IndexMetadata, Settings> settingsSupplier;
32+
33+
/**
34+
* Use this constructor when you want to update the index that ILM runs on with <i>constant</i> settings.
35+
*/
2836
public UpdateSettingsStep(StepKey key, StepKey nextStepKey, Client client, Settings settings) {
37+
this(key, nextStepKey, client, DEFAULT_TARGET_INDEX_NAME_SUPPLIER, indexMetadata -> settings);
38+
}
39+
40+
/**
41+
* 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
42+
* (i.e., settings that depend on the index metadata).
43+
*/
44+
public UpdateSettingsStep(
45+
StepKey key,
46+
StepKey nextStepKey,
47+
Client client,
48+
BiFunction<String, LifecycleExecutionState, String> targetIndexNameSupplier,
49+
Function<IndexMetadata, Settings> settingsSupplier
50+
) {
2951
super(key, nextStepKey, client);
30-
this.settings = settings;
52+
this.targetIndexNameSupplier = targetIndexNameSupplier;
53+
this.settingsSupplier = settingsSupplier;
3154
}
3255

3356
@Override
@@ -42,32 +65,16 @@ public void performAction(
4265
ClusterStateObserver observer,
4366
ActionListener<Void> listener
4467
) {
45-
UpdateSettingsRequest updateSettingsRequest = new UpdateSettingsRequest(indexMetadata.getIndex().getName()).masterNodeTimeout(
46-
TimeValue.MAX_VALUE
47-
).settings(settings);
68+
String indexName = targetIndexNameSupplier.apply(indexMetadata.getIndex().getName(), indexMetadata.getLifecycleExecutionState());
69+
Settings settings = settingsSupplier.apply(indexMetadata);
70+
UpdateSettingsRequest updateSettingsRequest = new UpdateSettingsRequest(indexName).masterNodeTimeout(TimeValue.MAX_VALUE)
71+
.settings(settings);
4872
getClient(currentState.projectId()).admin()
4973
.indices()
5074
.updateSettings(updateSettingsRequest, listener.delegateFailureAndWrap((l, r) -> l.onResponse(null)));
5175
}
5276

53-
public Settings getSettings() {
54-
return settings;
55-
}
56-
57-
@Override
58-
public int hashCode() {
59-
return Objects.hash(super.hashCode(), settings);
60-
}
61-
62-
@Override
63-
public boolean equals(Object obj) {
64-
if (obj == null) {
65-
return false;
66-
}
67-
if (getClass() != obj.getClass()) {
68-
return false;
69-
}
70-
UpdateSettingsStep other = (UpdateSettingsStep) obj;
71-
return super.equals(obj) && Objects.equals(settings, other.settings);
77+
public Function<IndexMetadata, Settings> getSettingsSupplier() {
78+
return settingsSupplier;
7279
}
7380
}

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/AllocateActionTests.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ public void testToSteps() {
186186
expectedSettings.put(ShardsLimitAllocationDecider.INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey(), action.getTotalShardsPerNode());
187187
}
188188

189-
assertThat(firstStep.getSettings(), equalTo(expectedSettings.build()));
189+
assertThat(firstStep.getSettingsSupplier().apply(null), equalTo(expectedSettings.build()));
190190
AllocationRoutedStep secondStep = (AllocationRoutedStep) steps.get(1);
191191
assertEquals(expectedSecondStepKey, secondStep.getKey());
192192
assertEquals(nextStepKey, secondStep.getNextStepKey());
@@ -204,13 +204,15 @@ public void testTotalNumberOfShards() throws Exception {
204204
);
205205
List<Step> steps = action.toSteps(null, phase, nextStepKey);
206206
UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(0);
207-
assertEquals(totalShardsPerNode, firstStep.getSettings().getAsInt(INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey(), null));
207+
Settings actualSettings = firstStep.getSettingsSupplier().apply(null);
208+
assertEquals(totalShardsPerNode, actualSettings.getAsInt(INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey(), null));
208209

209210
totalShardsPerNode = null;
210211
action = new AllocateAction(numberOfReplicas, totalShardsPerNode, null, null, null);
211212
steps = action.toSteps(null, phase, nextStepKey);
212213
firstStep = (UpdateSettingsStep) steps.get(0);
213-
assertEquals(null, firstStep.getSettings().get(INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey()));
214+
actualSettings = firstStep.getSettingsSupplier().apply(null);
215+
assertNull(actualSettings.get(INDEX_TOTAL_SHARDS_PER_NODE_SETTING.getKey()));
214216

215217
// allow an allocate action that only specifies total shards per node (don't expect any exceptions in this case)
216218
action = new AllocateAction(null, 5, null, null, null);

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/ForceMergeActionTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ private void assertBestCompression(ForceMergeAction instance) {
140140

141141
UpdateSettingsStep updateCodecStep = (UpdateSettingsStep) steps.get(5);
142142
assertThat(
143-
updateCodecStep.getSettings().get(EngineConfig.INDEX_CODEC_SETTING.getKey()),
143+
updateCodecStep.getSettingsSupplier().apply(null).get(EngineConfig.INDEX_CODEC_SETTING.getKey()),
144144
equalTo(CodecService.BEST_COMPRESSION_CODEC)
145145
);
146146
}

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/MigrateActionTests.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,17 +92,20 @@ public void testMigrateActionsConfiguresTierPreference() {
9292
{
9393
List<Step> steps = MigrateAction.ENABLED.toSteps(null, HOT_PHASE, nextStepKey);
9494
UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(1);
95-
assertThat(DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettings()), is(DATA_HOT));
95+
assertThat(DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettingsSupplier().apply(null)), is(DATA_HOT));
9696
}
9797
{
9898
List<Step> steps = MigrateAction.ENABLED.toSteps(null, WARM_PHASE, nextStepKey);
9999
UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(1);
100-
assertThat(DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettings()), is(DATA_WARM + "," + DATA_HOT));
100+
assertThat(DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettingsSupplier().apply(null)), is(DATA_WARM + "," + DATA_HOT));
101101
}
102102
{
103103
List<Step> steps = MigrateAction.ENABLED.toSteps(null, COLD_PHASE, nextStepKey);
104104
UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(1);
105-
assertThat(DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettings()), is(DATA_COLD + "," + DATA_WARM + "," + DATA_HOT));
105+
assertThat(
106+
DataTier.TIER_PREFERENCE_SETTING.get(firstStep.getSettingsSupplier().apply(null)),
107+
is(DATA_COLD + "," + DATA_WARM + "," + DATA_HOT)
108+
);
106109
}
107110
}
108111

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/SetPriorityActionTests.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,8 @@ public void testToSteps() {
6969
UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(0);
7070
assertThat(firstStep.getKey(), equalTo(expectedFirstStepKey));
7171
assertThat(firstStep.getNextStepKey(), equalTo(nextStepKey));
72-
assertThat(firstStep.getSettings().size(), equalTo(1));
73-
assertEquals(priority, (long) IndexMetadata.INDEX_PRIORITY_SETTING.get(firstStep.getSettings()));
72+
assertThat(firstStep.getSettingsSupplier().apply(null).size(), equalTo(1));
73+
assertEquals(priority, (long) IndexMetadata.INDEX_PRIORITY_SETTING.get(firstStep.getSettingsSupplier().apply(null)));
7474
}
7575

7676
public void testNullPriorityStep() {
@@ -88,10 +88,10 @@ public void testNullPriorityStep() {
8888
UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(0);
8989
assertThat(firstStep.getKey(), equalTo(expectedFirstStepKey));
9090
assertThat(firstStep.getNextStepKey(), equalTo(nextStepKey));
91-
assertThat(firstStep.getSettings().size(), equalTo(1));
91+
assertThat(firstStep.getSettingsSupplier().apply(null).size(), equalTo(1));
9292
assertThat(
93-
IndexMetadata.INDEX_PRIORITY_SETTING.get(firstStep.getSettings()),
94-
equalTo(IndexMetadata.INDEX_PRIORITY_SETTING.getDefault(firstStep.getSettings()))
93+
IndexMetadata.INDEX_PRIORITY_SETTING.get(firstStep.getSettingsSupplier().apply(null)),
94+
equalTo(IndexMetadata.INDEX_PRIORITY_SETTING.getDefault(firstStep.getSettingsSupplier().apply(null)))
9595
);
9696
}
9797

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStepTests.java‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,16 +32,14 @@ public UpdateSettingsStep createRandomInstance() {
3232
public UpdateSettingsStep mutateInstance(UpdateSettingsStep instance) {
3333
StepKey key = instance.getKey();
3434
StepKey nextKey = instance.getNextStepKey();
35-
Settings settings = instance.getSettings();
3635

37-
switch (between(0, 2)) {
36+
switch (between(0, 1)) {
3837
case 0 -> key = new StepKey(key.phase(), key.action(), key.name() + randomAlphaOfLength(5));
3938
case 1 -> nextKey = new StepKey(nextKey.phase(), nextKey.action(), nextKey.name() + randomAlphaOfLength(5));
40-
case 2 -> settings = Settings.builder().put(settings).put(randomAlphaOfLength(10), randomInt()).build();
4139
default -> throw new AssertionError("Illegal randomisation branch");
4240
}
4341

44-
return new UpdateSettingsStep(key, nextKey, client, settings);
42+
return new UpdateSettingsStep(key, nextKey, client, instance.getSettingsSupplier().apply(null));
4543
}
4644

4745
@Override
@@ -50,7 +48,7 @@ public UpdateSettingsStep copyInstance(UpdateSettingsStep instance) {
5048
instance.getKey(),
5149
instance.getNextStepKey(),
5250
instance.getClientWithoutProject(),
53-
instance.getSettings()
51+
instance.getSettingsSupplier().apply(null)
5452
);
5553
}
5654

@@ -71,7 +69,7 @@ public void testPerformAction() throws Exception {
7169
UpdateSettingsRequest request = (UpdateSettingsRequest) invocation.getArguments()[0];
7270
@SuppressWarnings("unchecked")
7371
ActionListener<AcknowledgedResponse> listener = (ActionListener<AcknowledgedResponse>) invocation.getArguments()[1];
74-
assertThat(request.settings(), equalTo(step.getSettings()));
72+
assertThat(request.settings(), equalTo(step.getSettingsSupplier().apply(indexMetadata)));
7573
assertThat(request.indices(), equalTo(new String[] { indexMetadata.getIndex().getName() }));
7674
listener.onResponse(AcknowledgedResponse.TRUE);
7775
return null;
@@ -96,7 +94,7 @@ public void testPerformActionFailure() {
9694
UpdateSettingsRequest request = (UpdateSettingsRequest) invocation.getArguments()[0];
9795
@SuppressWarnings("unchecked")
9896
ActionListener<AcknowledgedResponse> listener = (ActionListener<AcknowledgedResponse>) invocation.getArguments()[1];
99-
assertThat(request.settings(), equalTo(step.getSettings()));
97+
assertThat(request.settings(), equalTo(step.getSettingsSupplier().apply(indexMetadata)));
10098
assertThat(request.indices(), equalTo(new String[] { indexMetadata.getIndex().getName() }));
10199
listener.onFailure(exception);
102100
return null;

0 commit comments

Comments
 (0)