Skip to content

Make index.lifecycle.name setting internal #32518

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

Merged
merged 2 commits into from
Aug 2, 2018

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Jul 31, 2018

This commit makes the index.lifecycle.name setting internal an index, this
means that the policy can only be set on the index creation, or with the
specialized RestSetIndexLifecyclePolicy action.

Relates to #29823

@colings86 I'm not 100% sure we should do as-is. You currently can't remove a policy from the index, unlike a setting (which you could set to null to remove it) we don't currently have a way to do that, in order to delete a policy then you have to create a dummy policy and update the index to use it. Maybe someone can explain more of the background for why we originally wanted this to be an internal setting?

I think we should either add an API to remove the lifecycle policy from an index, or keep this as a dynamic setting, what do you think?

This commit makes the `index.lifecycle.name` setting internal an index, this
means that the policy can only be set on the index creation, or with the
specialized `RestSetIndexLifecyclePolicy` action.

Relates to elastic#29823
@dakrone dakrone added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Jul 31, 2018
@dakrone dakrone requested review from colings86 and talevy July 31, 2018 20:25
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@talevy
Copy link
Contributor

talevy commented Jul 31, 2018

Maybe someone can explain more of the background for why we originally wanted this to be an internal setting?

We did not preserve the current version of the policy that was mid-execution, so it was not always safe (see Shrink) to switch policies without extra validation and guards against potential issues.

That being said, latest meetings around this led to the new design goal laid out in the meta issue:

Store phase JSON in index metadata when an index moves to a new phase and use that version to drive execution until the end of the phase regardless of changes to the underlying policy (#29823)

The above state preservation would void any validation and guards since each index would remember what it is currently executing, so switching the policy underneath would not be a problem.

Let me know if that made sense @dakrone. We may not need this API at all anymore

@dakrone
Copy link
Member Author

dakrone commented Jul 31, 2018

I think that does make sense, in that case I'm extra-+1 on the solution where we preserved the state in the metadata so that we can completely remove this API.

@colings86 does that sound good? If so, I will work on removing the SetIndexLifecyclePolicy* stuff

@colings86
Copy link
Contributor

You currently can't remove a policy from the index

I'm not sure I follow here because we have RemovePolicyForIndexAction to remove a policy from an index?

does that sound good? If so, I will work on removing the SetIndexLifecyclePolicy* stuff

I think we should wait until we have done the work to store the current phase in the index metadata before we remove this API so we can make sure its definitely not needed anymore, from both a technical and non-technical standpoint

@dakrone
Copy link
Member Author

dakrone commented Aug 1, 2018

I'm not sure I follow here because we have RemovePolicyForIndexAction to remove a policy from an index?

Gah, I (erroneously) assumed that because all the tests were using the settings version that we didn't have an API for it.

I think we should wait until we have done the work to store the current phase in the index metadata before we remove this API so we can make sure its definitely not needed anymore, from both a technical and non-technical standpoint

Okay, I've changed the test to use the remove policy action, do we want to merge this for now and discuss removing it at a later time?

Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

LGTM

@dakrone dakrone merged commit 1bb7b53 into elastic:index-lifecycle Aug 2, 2018
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 2, 2018
…ecycle-underscore-x-pack

* elastic/index-lifecycle:
  Make index.lifecycle.name setting internal (elastic#32518)
jasontedor pushed a commit that referenced this pull request Aug 17, 2018
This commit makes the `index.lifecycle.name` setting internal an index, this
means that the policy can only be set on the index creation, or with the
specialized `RestSetIndexLifecyclePolicy` action.

Relates to #29823
@dakrone dakrone deleted the ilm-make-policy-internal branch February 4, 2019 14:46
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.

4 participants