Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes PhaseAfterStep to take the name of the previous phase #30756

Merged
merged 2 commits into from May 24, 2018
Merged

Changes PhaseAfterStep to take the name of the previous phase #30756

merged 2 commits into from May 24, 2018

Conversation

colings86
Copy link
Contributor

This changes the way the phase after step is built so its key has the
phase name of the phase that preceeds it rather than the phase that
follows it. This is more intuitive to the user since the index is in
the warm phase until the after condition for the cold phase is met.

@colings86 colings86 added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label May 21, 2018
@colings86 colings86 requested a review from talevy May 21, 2018 14:07
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@@ -203,7 +207,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
}

if (phase != null) {
Step.StepKey afterStepKey = new Step.StepKey(phase.getName(), "pre-" + lastStepKey.getAction(), "after");
// The very first after step is in a phase before the hot phase so call this "new"
Step.StepKey afterStepKey = new Step.StepKey("new", PhaseAfterStep.NAME, PhaseAfterStep.NAME);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, for documentation purposes, our phases are now

  1. new
  2. hot (optional)
  3. warm (optional)
  4. cold (optional)
  5. delete (optional)
  6. completed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, although we should be clear in the docs that new and completed are not real phases in the fact that you cannot add actions to them

Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are some assertions made in rest tests expecting the pre-pre-readonly step

https://github.com/talevy/elasticsearch/blob/ca9f307b0a72286aaa6595f2a0087c1ddd6f75c6/x-pack/plugin/src/test/resources/rest-api-spec/test/index_lifecycle/20_move_to_step.yml#L133-L135

this needs to be updated for tests to pass

@colings86
Copy link
Contributor Author

@talevy thanks for pointing that out. I've pushed a fix so hopefully the CI build will pass now

Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging latest index-lifecycle should fix CI on this PR

This changes the way the phase after step is built so its key has the
phase name of the phase that preceeds it rather than the phase that
follows it. This is more intuitive to the user since the index is in
the warm phase until the after condition for the cold phase is met.
x-pack/plugin/src/test/resources/rest-api-spec/test/index_lifecycle/20_m
ove_to_step.yml
x-pack/plugin/src/test/resources/rest-api-spec/test/index_lifecycle/20_m
ove_to_step.yml
@colings86 colings86 merged commit b308a4a into elastic:index-lifecycle May 24, 2018
@colings86 colings86 deleted the ilm/phaseAfterNaming branch May 24, 2018 13:01
jasontedor pushed a commit that referenced this pull request Aug 17, 2018
* Changes PhaseAfterStep to take the name of the previous phase

This changes the way the phase after step is built so its key has the
phase name of the phase that preceeds it rather than the phase that
follows it. This is more intuitive to the user since the index is in
the warm phase until the after condition for the cold phase is met.

* Fixes REST tests

x-pack/plugin/src/test/resources/rest-api-spec/test/index_lifecycle/20_m
ove_to_step.yml
x-pack/plugin/src/test/resources/rest-api-spec/test/index_lifecycle/20_m
ove_to_step.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/ILM+SLM Index and Snapshot lifecycle management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants