move phase-after steps to have the previous phase's phase in its key (elastic#4387)

talevy · web-flow · commit 689d628d67df · 2018-04-16T21:19:30.000-07:00
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).
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
@@ -12,6 +12,7 @@
 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
@@ -139,6 +139,32 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
         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<Step> toSteps(Client client, LongSupplier nowSupplier) {
         List<Step> steps = new ArrayList<>();
         List<Phase> orderedPhases = type.getOrderedPhases(phases);
@@ -148,9 +174,19 @@ public List<Step> toSteps(Client client, LongSupplier nowSupplier) {
         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<LifecycleAction> orderedActions = type.getOrderedActions(phase);
             ListIterator<LifecycleAction> actionIterator = orderedActions.listIterator(orderedActions.size());
             // add steps for each action, in reverse
@@ -164,23 +200,24 @@ public List<Step> toSteps(Client client, LongSupplier nowSupplier) {
                     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
@@ -15,6 +15,7 @@
 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.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<LifecyclePolicy> {
@@ -105,38 +107,55 @@ public void testDefaultLifecycleType() {
         assertSame(TimeseriesLifecycleType.INSTANCE, policy.getType());
     }
 
+    public void testFirstAndLastSteps() {
+        Client client = mock(Client.class);
+        LongSupplier nowSupplier = () -> 0L;
+        lifecycleName = randomAlphaOfLengthBetween(1, 20);
+        Map<String, Phase> phases = new LinkedHashMap<>();
+        LifecyclePolicy policy = new LifecyclePolicy(TestLifecycleType.INSTANCE, lifecycleName, phases);
+        List<Step> 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<String, Phase> phases = new LinkedHashMap<>();
-        LifecycleAction firstAction = new MockAction(Arrays.asList(firstStep));
+        LifecycleAction firstAction = new MockAction(Arrays.asList(mockStep));
         Map<String, LifecycleAction> 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<Step> 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<String, Phase> phases = new LinkedHashMap<>();
@@ -164,6 +183,6 @@ public void testToStepsWithTwoPhases() {
         assertThat(steps.get(4).getKey(), equalTo(secondAfter.getKey()));
         assertThat(steps.get(4).getNextStepKey(), equalTo(secondAfter.getNextStepKey()));
         assertThat(steps.get(5), equalTo(secondActionStep));
-        assertThat(steps.get(6).getClass(), equalTo(TerminalPolicyStep.class));
+        assertSame(steps.get(6), TerminalPolicyStep.INSTANCE);
     }
 }
