Skip to content

Commit 0783cd1

Browse files
nielsbaumannielsbauman
authored
Fix equality bug in WaitForIndexColorStep (#126605) (#126619)
The `indexNameSupplier` was included in the equality and is of type `BiFunction`, which doesn't implement a proper `equals` method by default - and thus neither do the lambdas. This meant that two instances of this step would only be considered equal if they were the same instance. By excluding `indexNameSupplier` from the `equals` method, we ensure the method works as intended and is able to properly tell the equality between two instances. As a side effect, we expect/hope this change will fix a number of tests that were failing because `WaitForIndexColorStep` missed the last cluster state update in the test, causing ILM to get stuck and the test to time out. Fixes #125683 Fixes #125789 Fixes #125867 Fixes #125911 Fixes #126053 Fixes #126354 (cherry picked from commit 3231eb2) # Conflicts: # muted-tests.yml # x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java
1 parent 3a990bc commit 0783cd1

File tree

3 files changed

+22
-37
lines changed

3 files changed

+22
-37
lines changed

‎docs/changelog/126605.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 126605
2+
summary: Fix equality bug in `WaitForIndexColorStep`
3+
area: ILM+SLM
4+
type: bug
5+
issues: []

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ BiFunction<String, LifecycleExecutionState, String> getIndexNameSupplier() {
6565

6666
@Override
6767
public int hashCode() {
68-
return Objects.hash(super.hashCode(), this.color, this.indexNameSupplier);
68+
return Objects.hash(super.hashCode(), this.color);
6969
}
7070

7171
@Override
@@ -77,9 +77,7 @@ public boolean equals(Object obj) {
7777
return false;
7878
}
7979
WaitForIndexColorStep other = (WaitForIndexColorStep) obj;
80-
return super.equals(obj)
81-
&& Objects.equals(this.color, other.color)
82-
&& Objects.equals(this.indexNameSupplier, other.indexNameSupplier);
80+
return super.equals(obj) && Objects.equals(this.color, other.color);
8381
}
8482

8583
@Override

‎x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java‎

Lines changed: 15 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -113,10 +113,11 @@ public void testSearchableSnapshotAction() throws Exception {
113113
}
114114
}, 30, TimeUnit.SECONDS));
115115

116-
assertBusy(() -> {
117-
triggerStateChange();
118-
assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME));
119-
}, 30, TimeUnit.SECONDS);
116+
assertBusy(
117+
() -> { assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME)); },
118+
30,
119+
TimeUnit.SECONDS
120+
);
120121
}
121122

122123
public void testSearchableSnapshotForceMergesIndexToOneSegment() throws Exception {
@@ -173,10 +174,11 @@ public void testSearchableSnapshotForceMergesIndexToOneSegment() throws Exceptio
173174
}
174175
}, 60, TimeUnit.SECONDS));
175176

176-
assertBusy(() -> {
177-
triggerStateChange();
178-
assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME));
179-
}, 30, TimeUnit.SECONDS);
177+
assertBusy(
178+
() -> { assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME)); },
179+
30,
180+
TimeUnit.SECONDS
181+
);
180182
}
181183

182184
@SuppressWarnings("unchecked")
@@ -314,7 +316,6 @@ public void testUpdatePolicyToAddPhasesYieldsInvalidActionsToBeSkipped() throws
314316
}, 30, TimeUnit.SECONDS));
315317

316318
assertBusy(() -> {
317-
triggerStateChange();
318319
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), restoredIndexName);
319320
assertThat(stepKeyForIndex.phase(), is("hot"));
320321
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -337,7 +338,6 @@ public void testUpdatePolicyToAddPhasesYieldsInvalidActionsToBeSkipped() throws
337338
// even though the index is now mounted as a searchable snapshot, the actions that can't operate on it should
338339
// skip and ILM should not be blocked (not should the managed index move into the ERROR step)
339340
assertBusy(() -> {
340-
triggerStateChange();
341341
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), restoredIndexName);
342342
assertThat(stepKeyForIndex.phase(), is("cold"));
343343
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -392,7 +392,6 @@ public void testRestoredIndexManagedByLocalPolicySkipsIllegalActions() throws Ex
392392
}, 30, TimeUnit.SECONDS));
393393

394394
assertBusy(() -> {
395-
triggerStateChange();
396395
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName);
397396
assertThat(stepKeyForIndex.phase(), is("hot"));
398397
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -483,7 +482,6 @@ public void testIdenticalSearchableSnapshotActionIsNoop() throws Exception {
483482
}, 30, TimeUnit.SECONDS);
484483

485484
assertBusy(() -> {
486-
triggerStateChange();
487485
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName);
488486
assertThat(stepKeyForIndex.phase(), is("cold"));
489487
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -545,7 +543,6 @@ public void testConvertingSearchableSnapshotFromFullToPartial() throws Exception
545543
}, 30, TimeUnit.SECONDS);
546544

547545
assertBusy(() -> {
548-
triggerStateChange();
549546
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName);
550547
assertThat(stepKeyForIndex.phase(), is("frozen"));
551548
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -628,7 +625,6 @@ public void testResumingSearchableSnapshotFromFullToPartial() throws Exception {
628625
}, 30, TimeUnit.SECONDS);
629626

630627
assertBusy(() -> {
631-
triggerStateChange();
632628
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), fullMountedIndexName);
633629
assertThat(stepKeyForIndex.phase(), is("cold"));
634630
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -649,7 +645,6 @@ public void testResumingSearchableSnapshotFromFullToPartial() throws Exception {
649645
}, 30, TimeUnit.SECONDS);
650646

651647
assertBusy(() -> {
652-
triggerStateChange();
653648
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), partiallyMountedIndexName);
654649
assertThat(stepKeyForIndex.phase(), is("frozen"));
655650
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -739,7 +734,6 @@ public void testResumingSearchableSnapshotFromPartialToFull() throws Exception {
739734
}, 30, TimeUnit.SECONDS);
740735

741736
assertBusy(() -> {
742-
triggerStateChange();
743737
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), partialMountedIndexName);
744738
assertThat(stepKeyForIndex.phase(), is("frozen"));
745739
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -760,7 +754,6 @@ public void testResumingSearchableSnapshotFromPartialToFull() throws Exception {
760754
}, 30, TimeUnit.SECONDS);
761755

762756
assertBusy(() -> {
763-
triggerStateChange();
764757
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), restoredPartiallyMountedIndexName);
765758
assertThat(stepKeyForIndex.phase(), is("cold"));
766759
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -920,10 +913,11 @@ public void testSearchableSnapshotInvokesAsyncActionOnNewIndex() throws Exceptio
920913
}
921914
}, 30, TimeUnit.SECONDS));
922915

923-
assertBusy(() -> {
924-
triggerStateChange();
925-
assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME));
926-
}, 30, TimeUnit.SECONDS);
916+
assertBusy(
917+
() -> { assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME)); },
918+
30,
919+
TimeUnit.SECONDS
920+
);
927921
}
928922

929923
public void testSearchableSnapshotTotalShardsPerNode() throws Exception {
@@ -964,7 +958,6 @@ public void testSearchableSnapshotTotalShardsPerNode() throws Exception {
964958
assertTrue(indexExists(searchableSnapMountedIndexName));
965959
}, 30, TimeUnit.SECONDS);
966960
assertBusy(() -> {
967-
triggerStateChange();
968961
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName);
969962
assertThat(stepKeyForIndex.phase(), is("frozen"));
970963
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -1028,7 +1021,6 @@ public void testSearchableSnapshotReplicateFor() throws Exception {
10281021

10291022
// check that the index is in the expected step and has the expected step_info.message
10301023
assertBusy(() -> {
1031-
triggerStateChange();
10321024
Map<String, Object> explainResponse = explainIndex(client(), restoredIndexName);
10331025
assertThat(explainResponse.get("step"), is(WaitUntilReplicateForTimePassesStep.NAME));
10341026
@SuppressWarnings("unchecked")
@@ -1066,7 +1058,6 @@ public void testSearchableSnapshotReplicateFor() throws Exception {
10661058

10671059
// check that the index has progressed because enough time has passed now that the policy is different
10681060
assertBusy(() -> {
1069-
triggerStateChange();
10701061
Map<String, Object> explainResponse = explainIndex(client(), restoredIndexName);
10711062
assertThat(explainResponse.get("phase"), is("cold"));
10721063
assertThat(explainResponse.get("step"), is(PhaseCompleteStep.NAME));
@@ -1080,13 +1071,4 @@ public void testSearchableSnapshotReplicateFor() throws Exception {
10801071
assertThat(numberOfReplicas, is(0));
10811072
}
10821073
}
1083-
1084-
/**
1085-
* Cause a bit of cluster activity using an empty reroute call in case the `wait-for-index-colour` ILM step missed the
1086-
* notification that partial-index is now GREEN.
1087-
*/
1088-
private void triggerStateChange() throws IOException {
1089-
Request rerouteRequest = new Request("POST", "/_cluster/reroute?metric=none");
1090-
client().performRequest(rerouteRequest);
1091-
}
10921074
}

0 commit comments

Comments
 (0)