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

[ilm] change remove-policy-from-index http method from DELETE to POST #35268

Merged
merged 2 commits into from
Nov 6, 2018

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Nov 5, 2018

The remove-ilm-from-index API was using the DELETE http method
to signify that something is being removed. Although, metadata
about ILM for the index is being deleted, no entity/resource
is being deleted during this operation. POST is more in line with
what this API is actually doing, it is modifying the metadata for
an index.

cc @bmcconaghy

The remove-ilm-from-index API was using the DELETE http method
to signify that something is being removed. Although, metadata
about ILM for the index is being deleted, no entity/resource
is being deleted during this operation. POST is more in line with
what this API is actually doing, it is modifying the metadata for
an index.
@talevy talevy added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Nov 5, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@talevy talevy changed the title [ilm] change remove policy http method from DELETE to POST [ilm] change remove-policy-from-index http method from DELETE to POST Nov 5, 2018
@talevy
Copy link
Contributor Author

talevy commented Nov 5, 2018

I believe that an additional URL path change should occur alongside the method change.

I'd like to use this PR to discuss.

One thing I would like to surface is:

POST _ilm

The above does not make it clear as to what is happening when called.

Should we add a literal remove to the path somewhere to make it clearer?

@dakrone
Copy link
Member

dakrone commented Nov 5, 2018

I definitely don't like it as just POST /foo/_ilm to remove the policy. I think at the very minimum we should be explicit and do POST /foo/_ilm/remove or POST /foo/_ilm/policy/remove (probably the former)

@talevy
Copy link
Contributor Author

talevy commented Nov 5, 2018

@dakrone, I'm in agreement (with the former). aka just appending /remove to the path 👍

@gwbrown
Copy link
Contributor

gwbrown commented Nov 5, 2018

++ for making this <index_name>/_ilm/remove

@talevy talevy added the stalled label Nov 5, 2018
@talevy talevy removed the stalled label Nov 6, 2018
@talevy
Copy link
Contributor Author

talevy commented Nov 6, 2018

thanks for providing your feedback @dakrone, @colings86, @gwbrown!

@talevy talevy merged commit a85b4f4 into elastic:master Nov 6, 2018
@talevy talevy deleted the ilm-post branch November 6, 2018 15:46
talevy added a commit that referenced this pull request Nov 6, 2018
…#35268)

The remove-ilm-from-index API was using the DELETE http method
to signify that something is being removed. Although, metadata
about ILM for the index is being deleted, no entity/resource
is being deleted during this operation. POST is more in line with
what this API is actually doing, it is modifying the metadata for
an index. As part of this change, `remove` is also appended to the path 
to be more explicit about its actions.
matarrese added a commit to matarrese/elasticsearch that referenced this pull request Nov 6, 2018
…-agg

* master: (528 commits)
  Register Azure max_retries setting (elastic#35286)
  add version 6.4.4
  [Docs] Add painless context details for bucket_script (elastic#35142)
  Upgrade jline to 3.8.2 (elastic#35288)
  SQL: new SQL CLI logo (elastic#35261)
  Logger: Merge ESLoggerFactory into Loggers (elastic#35146)
  Docs: Add section about range query for range type (elastic#35222)
  [ILM] change remove-policy-from-index http method from DELETE to POST (elastic#35268)
  [CCR] Forgot missing return statement,
  SQL: Fix null handling for AND and OR in SELECT (elastic#35277)
  [TEST] Mute ChangePolicyForIndexIT#testChangePolicyForIndex
  Serialize ignore_throttled also to 6.6 after backport
  Check for java 11 in buildSrc (elastic#35260)
  [TEST] increase await timeout in RemoteClusterConnectionTests
  Add missing up-to-date configuration (elastic#35255)
  Adapt Lucene BWC version
  SQL: Introduce Coalesce function (elastic#35253)
  Upgrade to lucene-8.0.0-snapshot-31d7dfe6b1 (elastic#35224)
  Fix failing ICU tests (elastic#35207)
  Prevent throttled indices to be searched through wildcards by default (elastic#34354)
  ...
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.

5 participants