Skip to content

Commit f515410

Browse files
nielsbaumannielsbauman
authored
Fix equality bug in WaitForIndexColorStep (#126605) (#126620)
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
1 parent 6e0473f commit f515410

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
@@ -114,10 +114,11 @@ public void testSearchableSnapshotAction() throws Exception {
114114
}
115115
}, 30, TimeUnit.SECONDS));
116116

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

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

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

183185
@SuppressWarnings("unchecked")
@@ -315,7 +317,6 @@ public void testUpdatePolicyToAddPhasesYieldsInvalidActionsToBeSkipped() throws
315317
}, 30, TimeUnit.SECONDS));
316318

317319
assertBusy(() -> {
318-
triggerStateChange();
319320
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), restoredIndexName);
320321
assertThat(stepKeyForIndex.phase(), is("hot"));
321322
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -338,7 +339,6 @@ public void testUpdatePolicyToAddPhasesYieldsInvalidActionsToBeSkipped() throws
338339
// even though the index is now mounted as a searchable snapshot, the actions that can't operate on it should
339340
// skip and ILM should not be blocked (not should the managed index move into the ERROR step)
340341
assertBusy(() -> {
341-
triggerStateChange();
342342
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), restoredIndexName);
343343
assertThat(stepKeyForIndex.phase(), is("cold"));
344344
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -394,7 +394,6 @@ public void testRestoredIndexManagedByLocalPolicySkipsIllegalActions() throws Ex
394394
}, 30, TimeUnit.SECONDS));
395395

396396
assertBusy(() -> {
397-
triggerStateChange();
398397
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName);
399398
assertThat(stepKeyForIndex.phase(), is("hot"));
400399
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -499,7 +498,6 @@ public void testIdenticalSearchableSnapshotActionIsNoop() throws Exception {
499498
}, 30, TimeUnit.SECONDS);
500499

501500
assertBusy(() -> {
502-
triggerStateChange();
503501
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName);
504502
assertThat(stepKeyForIndex.phase(), is("cold"));
505503
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -561,7 +559,6 @@ public void testConvertingSearchableSnapshotFromFullToPartial() throws Exception
561559
}, 30, TimeUnit.SECONDS);
562560

563561
assertBusy(() -> {
564-
triggerStateChange();
565562
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName);
566563
assertThat(stepKeyForIndex.phase(), is("frozen"));
567564
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -644,7 +641,6 @@ public void testResumingSearchableSnapshotFromFullToPartial() throws Exception {
644641
}, 30, TimeUnit.SECONDS);
645642

646643
assertBusy(() -> {
647-
triggerStateChange();
648644
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), fullMountedIndexName);
649645
assertThat(stepKeyForIndex.phase(), is("cold"));
650646
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -665,7 +661,6 @@ public void testResumingSearchableSnapshotFromFullToPartial() throws Exception {
665661
}, 30, TimeUnit.SECONDS);
666662

667663
assertBusy(() -> {
668-
triggerStateChange();
669664
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), partiallyMountedIndexName);
670665
assertThat(stepKeyForIndex.phase(), is("frozen"));
671666
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -755,7 +750,6 @@ public void testResumingSearchableSnapshotFromPartialToFull() throws Exception {
755750
}, 30, TimeUnit.SECONDS);
756751

757752
assertBusy(() -> {
758-
triggerStateChange();
759753
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), partialMountedIndexName);
760754
assertThat(stepKeyForIndex.phase(), is("frozen"));
761755
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -776,7 +770,6 @@ public void testResumingSearchableSnapshotFromPartialToFull() throws Exception {
776770
}, 30, TimeUnit.SECONDS);
777771

778772
assertBusy(() -> {
779-
triggerStateChange();
780773
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), restoredPartiallyMountedIndexName);
781774
assertThat(stepKeyForIndex.phase(), is("cold"));
782775
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -936,10 +929,11 @@ public void testSearchableSnapshotInvokesAsyncActionOnNewIndex() throws Exceptio
936929
}
937930
}, 30, TimeUnit.SECONDS));
938931

939-
assertBusy(() -> {
940-
triggerStateChange();
941-
assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME));
942-
}, 30, TimeUnit.SECONDS);
932+
assertBusy(
933+
() -> { assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME)); },
934+
30,
935+
TimeUnit.SECONDS
936+
);
943937
}
944938

945939
public void testSearchableSnapshotTotalShardsPerNode() throws Exception {
@@ -980,7 +974,6 @@ public void testSearchableSnapshotTotalShardsPerNode() throws Exception {
980974
assertTrue(indexExists(searchableSnapMountedIndexName));
981975
}, 30, TimeUnit.SECONDS);
982976
assertBusy(() -> {
983-
triggerStateChange();
984977
Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName);
985978
assertThat(stepKeyForIndex.phase(), is("frozen"));
986979
assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME));
@@ -1044,7 +1037,6 @@ public void testSearchableSnapshotReplicateFor() throws Exception {
10441037

10451038
// check that the index is in the expected step and has the expected step_info.message
10461039
assertBusy(() -> {
1047-
triggerStateChange();
10481040
Map<String, Object> explainResponse = explainIndex(client(), restoredIndexName);
10491041
assertThat(explainResponse.get("step"), is(WaitUntilReplicateForTimePassesStep.NAME));
10501042
@SuppressWarnings("unchecked")
@@ -1082,7 +1074,6 @@ public void testSearchableSnapshotReplicateFor() throws Exception {
10821074

10831075
// check that the index has progressed because enough time has passed now that the policy is different
10841076
assertBusy(() -> {
1085-
triggerStateChange();
10861077
Map<String, Object> explainResponse = explainIndex(client(), restoredIndexName);
10871078
assertThat(explainResponse.get("phase"), is("cold"));
10881079
assertThat(explainResponse.get("step"), is(PhaseCompleteStep.NAME));
@@ -1097,15 +1088,6 @@ public void testSearchableSnapshotReplicateFor() throws Exception {
10971088
}
10981089
}
10991090

1100-
/**
1101-
* Cause a bit of cluster activity using an empty reroute call in case the `wait-for-index-colour` ILM step missed the
1102-
* notification that partial-index is now GREEN.
1103-
*/
1104-
private void triggerStateChange() throws IOException {
1105-
Request rerouteRequest = new Request("POST", "/_cluster/reroute");
1106-
client().performRequest(rerouteRequest);
1107-
}
1108-
11091091
private Step.StepKey getKeyForIndex(Response response, String indexName) throws IOException {
11101092
Map<String, Object> responseMap;
11111093
try (InputStream is = response.getEntity().getContent()) {

0 commit comments

Comments
 (0)