Store phase steps for index in PolicyStepsRegistry by dakrone · Pull Request #32926 · elastic/elasticsearch

dakrone · 2018-08-16T21:12:37Z

This changes the way that steps are retrieved from PolicyStepsRegistry to
store the steps on a per-index basis (in memory for now, though that will change
in subsequent PRs). These steps are rebuilt as the index changes phases.

This also fixes a bug where an action with the same phase and name was not being
considered changed (and thus updated) in the compiled steps list. These are now
correctly considered as "upsert" diffs.

Relates to #29823

This changes the way that steps are retrieved from `PolicyStepsRegistry` to store the steps on a per-index basis (in memory for now, though that will change in subsequent PRs). These steps are rebuilt as the index changes phases. This also fixes a bug where an action with the same phase and name was not being considered changed (and thus updated) in the compiled steps list. These are now correctly considered as "upsert" diffs. Relates to elastic#29823

dakrone · 2018-08-16T21:13:21Z

...in/ilm/src/test/java/org/elasticsearch/xpack/indexlifecycle/ExecuteStepsUpdateTaskTests.java

        assertThat(LifecycleSettings.LIFECYCLE_STEP_INFO_SETTING.get(newState.metaData().index(index).getSettings()), equalTo(""));
    }

+    // TODO: I think these tests are invalid now that policy name is not used for retrieving the step


I'd love to hear feedback about these tests, whether they should be removed or what?

hmm. I think part of the problem is that I do not think we need a firstStepMap anymore? Since we can see our index is just initializing when we update the policy-registry and seed the indexPhaseSteps with the Steps from the first phase and just retrieve the first item in the list from there when asked for getFirstStep.

I'll investigate removing the firstStepMap, good idea :)

After investigating this, removing firstStepMap is much harder than it looks, so I'll leave that for a subsequent PR

dakrone · 2018-08-16T21:46:10Z

@elasticmachine test this please

talevy

looks great! I left a few questions and comments that I hope we can iterate on. We may find some not important, while others cause a few further internal changes to the PolicyStepsRegistry's data-structures

talevy · 2018-08-16T21:21:43Z

...gin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java

                actions.put(action.getWriteableName(), action);
            }
-            String phaseName = randomAlphaOfLength(10);
+            String phaseName = randomFrom(TimeseriesLifecycleType.VALID_PHASES);


is this required?

I don't think so any more, I'll revert

talevy · 2018-08-16T21:28:28Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistry.java


    @SuppressWarnings({ "unchecked", "rawtypes" })
-    public void update(IndexLifecycleMetadata meta, Client client, LongSupplier nowSupplier) {
+    public void update(ClusterState clusterState, IndexLifecycleMetadata meta, Client client, LongSupplier nowSupplier) {


should we break up this update method into its underlying parts of the MetaData that it is concerned with? Also fine with leaving it as is, just feels big and difficult to juggle which we're updating

I think that's a good idea, but not yet. The next parts of this refactor are going to be touching this method quite a bit, so I don't think it makes sense to split it up quite yet

talevy · 2018-08-16T21:31:50Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistry.java

+                    "_none_" : currentSteps.get(0).getKey().getPhase();
+                // Retrieve the current phase, defaulting to "new" if no phase is set
+                final String currentPhase = imd.value.getSettings().get(LifecycleSettings.LIFECYCLE_PHASE, "new");
+                assert currentPhase != null : "expected a current phase but it was null";


is this necessary given that the Settings.get with default will never have this be null?

I think this was a holdover from before I added the default return value for the setting retrieval, I'll remove it

talevy · 2018-08-16T21:45:48Z

...in/ilm/src/test/java/org/elasticsearch/xpack/indexlifecycle/ExecuteStepsUpdateTaskTests.java

        assertThat(LifecycleSettings.LIFECYCLE_STEP_INFO_SETTING.get(newState.metaData().index(index).getSettings()), equalTo(""));
    }

+    // TODO: I think these tests are invalid now that policy name is not used for retrieving the step


hmm. I think part of the problem is that I do not think we need a firstStepMap anymore? Since we can see our index is just initializing when we update the policy-registry and seed the indexPhaseSteps with the Steps from the first phase and just retrieve the first item in the list from there when asked for getFirstStep.

talevy · 2018-08-16T21:47:00Z

...in/ilm/src/test/java/org/elasticsearch/xpack/indexlifecycle/ExecuteStepsUpdateTaskTests.java


+    // TODO: I think these tests are invalid now that policy name is not used for retrieving the step
+    @AwaitsFix(bugUrl = "fix me")
    public void testExecuteInvalidStartStep() throws IOException {


I think this just needs to be made more general and not just test what it means to be an invalid first step, but an invalid step in general? I don't see other "invalid" input tests here and this was at least one of them

talevy · 2018-08-16T21:48:45Z

...in/ilm/src/test/java/org/elasticsearch/xpack/indexlifecycle/ExecuteStepsUpdateTaskTests.java

    }

+    @AwaitsFix(bugUrl = "fix me")
    public void testExecuteUntilNullStep() throws IOException {


I think the check that we hit the end of our Step.nextStepKey == null check can be added to the end of testExecuteAllUntilEndOfPhase

If we go that extra step in the execution in that test, we can get rid of this one

talevy

LGTM

Also switch to using Index rather than String for the map key, this means that a deleted and re-created index will not use the same phase policy steps as an old index.

dakrone · 2018-08-18T01:58:16Z

@elasticmachine test this please

…e-steps

…rack-phase-steps

* Store phase steps for index in PolicyStepsRegistry This changes the way that steps are retrieved from `PolicyStepsRegistry` to store the steps on a per-index basis (in memory for now, though that will change in subsequent PRs). These steps are rebuilt as the index changes phases. This also fixes a bug where an action with the same phase and name was not being considered changed (and thus updated) in the compiled steps list. These are now correctly considered as "upsert" diffs. Relates to #29823

dakrone added the :Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead. label Aug 16, 2018

dakrone requested review from colings86 and talevy August 16, 2018 21:12

dakrone commented Aug 16, 2018

View reviewed changes

elasticmachine mentioned this pull request Aug 16, 2018

[meta] Index Lifecycle Management Plan #29823

Closed

talevy reviewed Aug 16, 2018

View reviewed changes

dakrone added 4 commits August 16, 2018 16:58

Remove unneeded assert

19b5e46

revert back to random phase name

572de7f

Fix testExecuteAllUntilEndOfPhase

cb663a0

Fix testExecuteInvalidStartStep

8f0c402

talevy approved these changes Aug 17, 2018

View reviewed changes

dakrone added 2 commits August 17, 2018 13:12

Prevent NPE if the index hasn't been created yet on execution

325c39c

Remove delete indices from in-memory map

03c8a40

Also switch to using Index rather than String for the map key, this means that a deleted and re-created index will not use the same phase policy steps as an old index.

dakrone added 2 commits August 17, 2018 20:10

Merge remote-tracking branch 'origin/master' into ilm-only-track-phas…

12502fd

…e-steps

Merge remote-tracking branch 'origin/index-lifecycle' into ilm-only-t…

68c1a5e

…rack-phase-steps

dakrone merged commit 77016ad into elastic:index-lifecycle Aug 18, 2018

dakrone deleted the ilm-only-track-phase-steps branch February 4, 2019 14:45

Conversation

dakrone commented Aug 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakrone commented Aug 16, 2018

Uh oh!

talevy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talevy left a comment

Choose a reason for hiding this comment

Uh oh!

dakrone commented Aug 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants