From c2329cdf1dd89cf7cb04413b211bd19f11ec130a Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Tue, 27 Nov 2018 11:03:47 -0800 Subject: [PATCH] [ILM] reduce time restriction on IndexLifecycleExplainResponse (#35954) step times were set. The assumption was that these are always set. Tests passed, which led me to believe this was true. There is a time when shrunk indices have their step phase/action/step details set, but with no time information (in the CopyExecutionStateStep). Explain API fails for these --- .../IndexLifecycleExplainResponse.java | 9 ++---- .../IndexLifecycleExplainResponseTests.java | 8 ++--- .../IndexLifecycleExplainResponse.java | 9 ++---- .../IndexLifecycleExplainResponseTests.java | 30 +++++++++++-------- 4 files changed, 28 insertions(+), 28 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/indexlifecycle/IndexLifecycleExplainResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/indexlifecycle/IndexLifecycleExplainResponse.java index f79639e2443..8bdc3b195ac 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/indexlifecycle/IndexLifecycleExplainResponse.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/indexlifecycle/IndexLifecycleExplainResponse.java @@ -124,15 +124,12 @@ public class IndexLifecycleExplainResponse implements ToXContentObject { throw new IllegalArgumentException("[" + POLICY_NAME_FIELD.getPreferredName() + "] cannot be null for managed index"); } // check to make sure that step details are either all null or all set. - long numNull = Stream.of(phase, action, step, phaseTime, actionTime, stepTime).filter(Objects::isNull).count(); - if (numNull > 0 && numNull < 6) { + long numNull = Stream.of(phase, action, step).filter(Objects::isNull).count(); + if (numNull > 0 && numNull < 3) { throw new IllegalArgumentException("managed index response must have complete step details [" + PHASE_FIELD.getPreferredName() + "=" + phase + ", " + - PHASE_TIME_FIELD.getPreferredName() + "=" + phaseTime + ", " + ACTION_FIELD.getPreferredName() + "=" + action + ", " + - ACTION_TIME_FIELD.getPreferredName() + "=" + actionTime + ", " + - STEP_FIELD.getPreferredName() + "=" + step + ", " + - STEP_TIME_FIELD.getPreferredName() + "=" + stepTime + "]"); + STEP_FIELD.getPreferredName() + "=" + step + "]"); } } else { if (policyName != null || lifecycleDate != null || phase != null || action != null || step != null || failedStep != null diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/indexlifecycle/IndexLifecycleExplainResponseTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/indexlifecycle/IndexLifecycleExplainResponseTests.java index 40d9f7e194a..29f7a8db89f 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/indexlifecycle/IndexLifecycleExplainResponseTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/indexlifecycle/IndexLifecycleExplainResponseTests.java @@ -69,7 +69,7 @@ public class IndexLifecycleExplainResponseTests extends AbstractXContentTestCase } public void testInvalidStepDetails() { - final int numNull = randomIntBetween(1, 6); + final int numNull = randomIntBetween(1, 3); IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> IndexLifecycleExplainResponse.newManagedIndexResponse(randomAlphaOfLength(10), randomAlphaOfLength(10), @@ -78,9 +78,9 @@ public class IndexLifecycleExplainResponseTests extends AbstractXContentTestCase (numNull == 2) ? null : randomAlphaOfLength(10), (numNull == 3) ? null : randomAlphaOfLength(10), randomBoolean() ? null : randomAlphaOfLength(10), - (numNull == 4) ? null : randomNonNegativeLong(), - (numNull == 5) ? null : randomNonNegativeLong(), - (numNull == 6) ? null : randomNonNegativeLong(), + randomBoolean() ? null : randomNonNegativeLong(), + randomBoolean() ? null : randomNonNegativeLong(), + randomBoolean() ? null : randomNonNegativeLong(), randomBoolean() ? null : new BytesArray(new RandomStepInfo(() -> randomAlphaOfLength(10)).toString()), randomBoolean() ? null : PhaseExecutionInfoTests.randomPhaseExecutionInfo(""))); assertThat(exception.getMessage(), startsWith("managed index response must have complete step details")); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/IndexLifecycleExplainResponse.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/IndexLifecycleExplainResponse.java index 7880703d8ba..fd171c88539 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/IndexLifecycleExplainResponse.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/IndexLifecycleExplainResponse.java @@ -113,15 +113,12 @@ public class IndexLifecycleExplainResponse implements ToXContentObject, Writeabl throw new IllegalArgumentException("[" + POLICY_NAME_FIELD.getPreferredName() + "] cannot be null for managed index"); } // check to make sure that step details are either all null or all set. - long numNull = Stream.of(phase, action, step, phaseTime, actionTime, stepTime).filter(Objects::isNull).count(); - if (numNull > 0 && numNull < 6) { + long numNull = Stream.of(phase, action, step).filter(Objects::isNull).count(); + if (numNull > 0 && numNull < 3) { throw new IllegalArgumentException("managed index response must have complete step details [" + PHASE_FIELD.getPreferredName() + "=" + phase + ", " + - PHASE_TIME_FIELD.getPreferredName() + "=" + phaseTime + ", " + ACTION_FIELD.getPreferredName() + "=" + action + ", " + - ACTION_TIME_FIELD.getPreferredName() + "=" + actionTime + ", " + - STEP_FIELD.getPreferredName() + "=" + step + ", " + - STEP_TIME_FIELD.getPreferredName() + "=" + stepTime + "]"); + STEP_FIELD.getPreferredName() + "=" + step + "]"); } } else { if (policyName != null || lifecycleDate != null || phase != null || action != null || step != null || failedStep != null diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/IndexLifecycleExplainResponseTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/IndexLifecycleExplainResponseTests.java index 6381dd1ae41..4b483dcf039 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/IndexLifecycleExplainResponseTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/IndexLifecycleExplainResponseTests.java @@ -60,7 +60,7 @@ public class IndexLifecycleExplainResponseTests extends AbstractSerializingTestC } public void testInvalidStepDetails() { - final int numNull = randomIntBetween(1, 6); + final int numNull = randomIntBetween(1, 3); IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> IndexLifecycleExplainResponse.newManagedIndexResponse(randomAlphaOfLength(10), randomAlphaOfLength(10), @@ -69,9 +69,9 @@ public class IndexLifecycleExplainResponseTests extends AbstractSerializingTestC (numNull == 2) ? null : randomAlphaOfLength(10), (numNull == 3) ? null : randomAlphaOfLength(10), randomBoolean() ? null : randomAlphaOfLength(10), - (numNull == 4) ? null : randomNonNegativeLong(), - (numNull == 5) ? null : randomNonNegativeLong(), - (numNull == 6) ? null : randomNonNegativeLong(), + randomBoolean() ? null : randomNonNegativeLong(), + randomBoolean() ? null : randomNonNegativeLong(), + randomBoolean() ? null : randomNonNegativeLong(), randomBoolean() ? null : new BytesArray(new RandomStepInfo(() -> randomAlphaOfLength(10)).toString()), randomBoolean() ? null : PhaseExecutionInfoTests.randomPhaseExecutionInfo(""))); assertThat(exception.getMessage(), startsWith("managed index response must have complete step details")); @@ -109,7 +109,7 @@ public class IndexLifecycleExplainResponseTests extends AbstractSerializingTestC BytesReference stepInfo = instance.getStepInfo(); PhaseExecutionInfo phaseExecutionInfo = instance.getPhaseExecutionInfo(); if (managed) { - switch (between(0, 7)) { + switch (between(0, 10)) { case 0: index = index + randomAlphaOfLengthBetween(1, 5); break; @@ -120,11 +120,17 @@ public class IndexLifecycleExplainResponseTests extends AbstractSerializingTestC phase = randomAlphaOfLengthBetween(1, 5); action = randomAlphaOfLengthBetween(1, 5); step = randomAlphaOfLengthBetween(1, 5); - phaseTime = randomValueOtherThan(phaseTime, () -> randomLongBetween(0, 100000)); - actionTime = randomValueOtherThan(actionTime, () -> randomLongBetween(0, 100000)); - stepTime = randomValueOtherThan(stepTime, () -> randomLongBetween(0, 100000)); break; case 3: + phaseTime = randomValueOtherThan(phaseTime, () -> randomLongBetween(0, 100000)); + break; + case 4: + actionTime = randomValueOtherThan(actionTime, () -> randomLongBetween(0, 100000)); + break; + case 5: + stepTime = randomValueOtherThan(stepTime, () -> randomLongBetween(0, 100000)); + break; + case 6: if (Strings.hasLength(failedStep) == false) { failedStep = randomAlphaOfLength(10); } else if (randomBoolean()) { @@ -133,10 +139,10 @@ public class IndexLifecycleExplainResponseTests extends AbstractSerializingTestC failedStep = null; } break; - case 4: + case 7: policyTime = randomValueOtherThan(policyTime, () -> randomLongBetween(0, 100000)); break; - case 5: + case 8: if (Strings.hasLength(stepInfo) == false) { stepInfo = new BytesArray(randomByteArrayOfLength(100)); } else if (randomBoolean()) { @@ -146,10 +152,10 @@ public class IndexLifecycleExplainResponseTests extends AbstractSerializingTestC stepInfo = null; } break; - case 6: + case 9: phaseExecutionInfo = randomValueOtherThan(phaseExecutionInfo, () -> PhaseExecutionInfoTests.randomPhaseExecutionInfo("")); break; - case 7: + case 10: return IndexLifecycleExplainResponse.newUnmanagedIndexResponse(index); default: throw new AssertionError("Illegal randomisation branch");