From 689d628d67df02bfed08673fbe7d95a2497fa315 Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Mon, 16 Apr 2018 21:19:30 -0700 Subject: [PATCH] move phase-after steps to have the previous phase's phase in its key (#4387) Previously, phase X's `after` step had `X` as its associated phase. This causes confusion because we have only entered phase `X` once the `after` step is complete. Therefore, this refactor pushes the after's phase to be associated with the previous phase. This first phase is an exception. The first phase's `after` step is associated with the first phase (not some non-existent prior phase). --- .../InitializePolicyContextStep.java | 1 + .../core/indexlifecycle/LifecyclePolicy.java | 47 +++++++++++++++-- .../indexlifecycle/LifecyclePolicyTests.java | 51 +++++++++++++------ 3 files changed, 78 insertions(+), 21 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/InitializePolicyContextStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/InitializePolicyContextStep.java index 4aaf5e77c40..4516c4b463b 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/InitializePolicyContextStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/InitializePolicyContextStep.java @@ -12,6 +12,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.Index; public class InitializePolicyContextStep extends Step { + public static final StepKey KEY = new StepKey("pre-phase", "pre-action", "init"); public InitializePolicyContextStep(Step.StepKey key, StepKey nextStepKey) { super(key, nextStepKey); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java index dbf90cfae66..379cd6f62f7 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java @@ -139,6 +139,32 @@ public class LifecyclePolicy extends AbstractDiffable return builder; } + /** + * This method is used to compile this policy into its execution plan built out + * of {@link Step} instances. The order of the {@link Phase}s and {@link LifecycleAction}s is + * determined by the {@link LifecycleType} associated with this policy. + * + * The order of the policy will have this structure: + * + * - initialize policy context step + * - phase-1 phase-after-step + * - ... phase-1 action steps + * - phase-2 phase-after-step + * - ... + * - terminal policy step + * + * We first initialize the policy's context and ensure that the index has proper settings set. + * Then we begin each phase's after-step along with all its actions as steps. Finally, we have + * a terminal step to inform us that this policy's steps are all complete. Each phase's `after` + * step is associated with the previous phase's phase. For example, the warm phase's `after` is + * associated with the hot phase so that it is clear that we haven't stepped into the warm phase + * just yet (until this step is complete). + * + * @param client The Elasticsearch Client to use during execution of {@link AsyncActionStep} + * and {@link AsyncWaitStep} steps. + * @param nowSupplier The supplier of the current time for {@link PhaseAfterStep} steps. + * @return The list of {@link Step} objects in order of their execution. + */ public List toSteps(Client client, LongSupplier nowSupplier) { List steps = new ArrayList<>(); List orderedPhases = type.getOrderedPhases(phases); @@ -148,9 +174,19 @@ public class LifecyclePolicy extends AbstractDiffable steps.add(TerminalPolicyStep.INSTANCE); Step.StepKey lastStepKey = TerminalPolicyStep.KEY; + Phase phase = null; // add steps for each phase, in reverse while (phaseIterator.hasPrevious()) { - Phase phase = phaseIterator.previous(); + + // add `after` step for phase before next + if (phase != null) { + Step.StepKey afterStepKey = new Step.StepKey(phase.getName(), "pre-" + lastStepKey.getAction(), "after"); + Step phaseAfterStep = new PhaseAfterStep(nowSupplier, phase.getAfter(), afterStepKey, lastStepKey); + steps.add(phaseAfterStep); + lastStepKey = phaseAfterStep.getKey(); + } + + phase = phaseIterator.previous(); List orderedActions = type.getOrderedActions(phase); ListIterator actionIterator = orderedActions.listIterator(orderedActions.size()); // add steps for each action, in reverse @@ -164,23 +200,24 @@ public class LifecyclePolicy extends AbstractDiffable lastStepKey = step.getKey(); } } + } - // add `after` step for phase - Step.StepKey afterStepKey = new Step.StepKey(phase.getName(), "pre-action", "after"); + if (phase != null) { + Step.StepKey afterStepKey = new Step.StepKey(phase.getName(), "pre-" + lastStepKey.getAction(), "after"); Step phaseAfterStep = new PhaseAfterStep(nowSupplier, phase.getAfter(), afterStepKey, lastStepKey); steps.add(phaseAfterStep); lastStepKey = phaseAfterStep.getKey(); } // init step so that policy is guaranteed to have - steps.add(new InitializePolicyContextStep( - new Step.StepKey("pre-phase", "pre-action", "init"), lastStepKey)); + steps.add(new InitializePolicyContextStep(InitializePolicyContextStep.KEY, lastStepKey)); Collections.reverse(steps); logger.debug("STEP COUNT: " + steps.size()); for (Step step : steps) { logger.debug(step.getKey() + " -> " + step.getNextStepKey()); } + return steps; } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java index e4eb882dbc3..40c69e13b71 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java @@ -15,6 +15,7 @@ import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.AbstractSerializingTestCase; +import org.elasticsearch.xpack.core.indexlifecycle.Step.StepKey; import java.io.IOException; import java.util.ArrayList; @@ -27,6 +28,7 @@ import java.util.Map; import java.util.function.LongSupplier; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; import static org.mockito.Mockito.mock; public class LifecyclePolicyTests extends AbstractSerializingTestCase { @@ -105,38 +107,55 @@ public class LifecyclePolicyTests extends AbstractSerializingTestCase 0L; + lifecycleName = randomAlphaOfLengthBetween(1, 20); + Map phases = new LinkedHashMap<>(); + LifecyclePolicy policy = new LifecyclePolicy(TestLifecycleType.INSTANCE, lifecycleName, phases); + List steps = policy.toSteps(client, nowSupplier); + assertThat(steps.size(), equalTo(2)); + assertThat(steps.get(0), instanceOf(InitializePolicyContextStep.class)); + assertThat(steps.get(0).getKey(), equalTo(new StepKey("pre-phase", "pre-action", "init"))); + assertThat(steps.get(0).getNextStepKey(), equalTo(TerminalPolicyStep.KEY)); + assertSame(steps.get(1), TerminalPolicyStep.INSTANCE); + } + public void testToStepsWithOneStep() { Client client = mock(Client.class); LongSupplier nowSupplier = () -> 0L; - MockStep firstStep = new MockStep(new Step.StepKey("test", "test", "test"), null); + MockStep mockStep = new MockStep( + new Step.StepKey("test", "test", "test"), TerminalPolicyStep.KEY); lifecycleName = randomAlphaOfLengthBetween(1, 20); Map phases = new LinkedHashMap<>(); - LifecycleAction firstAction = new MockAction(Arrays.asList(firstStep)); + LifecycleAction firstAction = new MockAction(Arrays.asList(mockStep)); Map actions = Collections.singletonMap(MockAction.NAME, firstAction); Phase firstPhase = new Phase("test", TimeValue.ZERO, actions); phases.put(firstPhase.getName(), firstPhase); LifecyclePolicy policy = new LifecyclePolicy(TestLifecycleType.INSTANCE, lifecycleName, phases); - + StepKey firstStepKey = InitializePolicyContextStep.KEY; + StepKey secondStepKey = new StepKey("test", "pre-test", "after"); List steps = policy.toSteps(client, nowSupplier); assertThat(steps.size(), equalTo(4)); - assertThat(steps.get(0).getKey(), equalTo(new Step.StepKey("pre-phase", "pre-action", "init"))); - assertThat(steps.get(0).getNextStepKey(), equalTo(new Step.StepKey("test", "pre-action", "after"))); - assertThat(steps.get(1).getKey(), equalTo(new Step.StepKey("test", "pre-action", "after"))); - assertThat(steps.get(1).getNextStepKey(), equalTo(firstStep.getKey())); - assertThat(steps.get(2), equalTo(firstStep)); - assertNull(steps.get(2).getNextStepKey()); + assertSame(steps.get(0).getKey(), firstStepKey); + assertThat(steps.get(0).getNextStepKey(), equalTo(secondStepKey)); + assertThat(steps.get(1).getKey(), equalTo(secondStepKey)); + assertThat(steps.get(1).getNextStepKey(), equalTo(mockStep.getKey())); + assertThat(steps.get(2).getKey(), equalTo(mockStep.getKey())); + assertThat(steps.get(2).getNextStepKey(), equalTo(TerminalPolicyStep.KEY)); + assertSame(steps.get(3), TerminalPolicyStep.INSTANCE); } public void testToStepsWithTwoPhases() { Client client = mock(Client.class); LongSupplier nowSupplier = () -> 0L; - MockStep secondActionStep = new MockStep(new Step.StepKey("second_phase", "test", "test"), null); - MockStep secondAfter = new MockStep(new Step.StepKey("second_phase", "pre-action", "after"), secondActionStep.getKey()); - MockStep firstActionAnotherStep = new MockStep(new Step.StepKey("first_phase", "test", "test"), secondAfter.getKey()); - MockStep firstActionStep = new MockStep(new Step.StepKey("first_phase", "test", "test"), firstActionAnotherStep.getKey()); - MockStep firstAfter = new MockStep(new Step.StepKey("first_phase", "pre-action", "after"), firstActionStep.getKey()); - MockStep init = new MockStep(new Step.StepKey("pre-phase", "pre-action", "init"), firstAfter.getKey()); + MockStep secondActionStep = new MockStep(new StepKey("second_phase", "test2", "test"), TerminalPolicyStep.KEY); + MockStep secondAfter = new MockStep(new StepKey("second_phase", "pre-test2", "after"), secondActionStep.getKey()); + MockStep firstActionAnotherStep = new MockStep(new StepKey("first_phase", "test", "bar"), secondAfter.getKey()); + MockStep firstActionStep = new MockStep(new StepKey("first_phase", "test", "foo"), firstActionAnotherStep.getKey()); + MockStep firstAfter = new MockStep(new StepKey("first_phase", "pre-test", "after"), firstActionStep.getKey()); + MockStep init = new MockStep(InitializePolicyContextStep.KEY, firstAfter.getKey()); lifecycleName = randomAlphaOfLengthBetween(1, 20); Map phases = new LinkedHashMap<>(); @@ -164,6 +183,6 @@ public class LifecyclePolicyTests extends AbstractSerializingTestCase