Skip to content

Removes Set Policy API in favour of setting index.lifecycle.name directly #34304

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 24, 2018
Merged

Removes Set Policy API in favour of setting index.lifecycle.name directly #34304

merged 7 commits into from
Oct 24, 2018

Conversation

@colings86
Copy link
Contributor

@colings86 colings86 commented Oct 4, 2018 •
edited
Loading

Still to do:

  • Add a test for changing the policy using the update settings API and checking the new policy is used after the current phase completes

Closes #34302

@colings86 colings86 added WIP :Data Management/ILM+SLM Index and Snapshot lifecycle management labels Oct 4, 2018
@colings86 colings86 self-assigned this Oct 4, 2018
@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 4, 2018

Pinging @elastic/es-core-infra

@colings86 colings86 mentioned this pull request Oct 4, 2018
Closed
@gwbrown gwbrown mentioned this pull request Oct 4, 2018
Closed
@dakrone
Copy link
Member

dakrone commented Oct 4, 2018

If we're doing this in the setting, I think we should remove the RemovePolicyForIndexAction also, and have a listener on the index.lifecycle.name setting that does what we need as "teardown" for an index when the setting is removed (set to null).

We could do it as a followup though. What do you think?

gwbrown
gwbrown reviewed Oct 4, 2018
View reviewed changes
Copy link
Contributor

@gwbrown gwbrown left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A couple minor comments, otherwise LGTM (though I second @dakrone's comment, and pending tests passing).

Also, ++ for +9 −1,884.

...ugin/ilm/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunnerTests.java Outdated
Copy link
Contributor

@gwbrown gwbrown Oct 4, 2018

Choose a reason for hiding this comment

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

This class is being used by the code in #34206

Copy link
Contributor Author

@colings86 colings86 Oct 4, 2018

Choose a reason for hiding this comment

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

Ok, I'll reinstate it then

client/rest-high-level/src/test/java/org/elasticsearch/client/IndexLifecycleIT.java Outdated
Copy link
Contributor

@gwbrown gwbrown Oct 4, 2018

Choose a reason for hiding this comment

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

Might be worth replacing this test case with one that makes sure changing the lifecycle policy via the settings API works as expected (i.e. actually runs the new policy)

Copy link
Contributor Author

@colings86 colings86 Oct 4, 2018

Choose a reason for hiding this comment

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

Agreed, that was the intention of the outstanding task in the description

Copy link
Contributor

@gwbrown gwbrown Oct 4, 2018

Choose a reason for hiding this comment

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

My bad, I misread that task.

Copy link
Contributor Author

@colings86 colings86 Oct 4, 2018

Choose a reason for hiding this comment

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

NP. My intention was to have this as a server side test rather than a test in the HLRC because its testing ILM functionality rather than the client or the API. Does that work for you or do you feel strongly this should be tested as part of the client?

Copy link
Contributor

@gwbrown gwbrown Oct 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I’m good with testing it server side.

@colings86
Copy link
Contributor Author

colings86 commented Oct 4, 2018

If we're doing this in the setting, I think we should remove the RemovePolicyForIndexAction also, and have a listener on the index.lifecycle.name setting that does what we need as "teardown" for an index when the setting is removed (set to null).

I agree this would be a good thing to do but I see it as lower priority given that the remove policy API works currently whereas this API does not. SO my preference would to do this as a subsequent change and for it to not be a blocker

@colings86 colings86 mentioned this pull request Oct 4, 2018
Closed
@colings86
Copy link
Contributor Author

colings86 commented Oct 4, 2018

I opened #34310 for the remove policy API

dakrone
dakrone reviewed Oct 4, 2018
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM

@colings86
Copy link
Contributor Author

colings86 commented Oct 16, 2018

@talevy @gwbrown I added the test mentioned int he description, could you take a look?

@colings86 colings86 removed the WIP label Oct 16, 2018
@talevy
Copy link
Contributor

talevy commented Oct 16, 2018

@colings86 yup, will take a look today. hopefully the conflicting files are minimal

talevy
talevy reviewed Oct 17, 2018
View reviewed changes
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.

I took a look at it, overall looks good to me!

the only reason I didn't approve is because this still has some conflicts with upstream.

@colings86
Copy link
Contributor Author

colings86 commented Oct 17, 2018

@elasticmachine test this please

1 similar comment
@colings86
Copy link
Contributor Author

colings86 commented Oct 17, 2018

@elasticmachine test this please

talevy
talevy approved these changes Oct 19, 2018
View reviewed changes
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.

LGTM

@colings86 colings86 merged commit c7fe87e into elastic:index-lifecycle Oct 24, 2018
colings86 added a commit that referenced this pull request Oct 24, 2018
@colings86
Removes Set Policy API in favour of setting index.lifecycle.name
84713f4 
directly (#34304)

* Removes Set Policy API in favour of setting index.lifecycle.name
directly

* Reinstates matcher that will still be used

* Cleans up code after rebase

* Adds test to check changing policy with ndex settings works

* Fixes TimeseriesLifecycleActionsIT after API removal

* Fixes docs tests

* Fixes case on close where lifecycle service was never created
@colings86 colings86 deleted the ilm/remove-set-policy-api branch October 24, 2018 15:17
talevy added a commit to talevy/elasticsearch that referenced this pull request Oct 24, 2018
@talevy
remove ILM API remove_policy
cf6969c 
Since elastic#34304, `index.lifecycle.name` is no longer an
internal index setting. This was done to simplify the API
and remove the dedicated API for setting a policy on an index
after it was created. The idea is that this is best done as an
index setting update. The same could be said about the remove_policy
API.

This commit removes the dedicated API command to remove a policy
from an index. Update Settings API should be used instead to
set `index.lifecycle.name` to null or the empty string.
@talevy talevy mentioned this pull request Oct 24, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dakrone dakrone dakrone left review comments

+2 more reviewers

@gwbrown gwbrown gwbrown left review comments

@talevy talevy talevy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@colings86 colings86

Labels

:Data Management/ILM+SLM Index and Snapshot lifecycle management

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@colings86 @elasticmachine @dakrone @talevy @gwbrown