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

Remove endpoint for freezing indices #77273

Closed

Conversation

danhermann
Copy link
Contributor

Relates to #70192

@danhermann danhermann added >breaking :Data Management/Indices APIs APIs to create and manage indices and templates v8.0.0 labels Sep 3, 2021
@@ -1533,24 +1530,6 @@ public void testAnalyze() throws Exception {
assertNotNull(detailsResponse.detail());
}

public void testFreezeAndUnfreeze() throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dakrone, one of consequences of removing the freeze endpoint is that many tests for unfreeze depend on it. Do you have any concerns about removing tests that exercise the unfreeze endpoint?

Copy link
Member

Choose a reason for hiding this comment

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

Not for these tests specifically, I think it's fine to change the client tests

Copy link
Contributor Author

@danhermann danhermann left a comment

Choose a reason for hiding this comment

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

@jrodewig, it looks like docs for the ES clients in other repos are failing because they reference the now-removed freeze index API? If you can confirm that, I will open PRs in the client repos to remove those references unless there is a better way of doing that.

@sethmlarson sethmlarson added the Team:Clients Meta label for clients team label Sep 7, 2021
@danhermann danhermann marked this pull request as ready for review September 7, 2021 12:12
@elasticmachine
Copy link
Collaborator

Pinging @elastic/clients-team (Team:Clients)

@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Sep 7, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@jakelandis
Copy link
Contributor

@danhermann - in prior discussions we decided to let REST API compatibility return a more meaningful error (see #70192 (comment)) if trying to freeze an index. Is that something that should be part of this PR, or a different on e ? cc @joegallo

@danhermann
Copy link
Contributor Author

@danhermann - in prior discussions we decided to let REST API compatibility return a more meaningful error (see #70192 (comment)) if trying to freeze an index. Is that something that should be part of this PR, or a different on e ? cc @joegallo

Thanks, @jakelandis. I'm happy to add that. Is there any documentation on how that works when an endpoint is completely removed as in this case?

@jrodewig
Copy link
Contributor

jrodewig commented Sep 8, 2021

@elasticmachine update branch

@elasticmachine
Copy link
Collaborator

merge conflict between base and head

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

Thanks for this update @danhermann. I pushed a commit to add some missing redirects. Once the merge conflict is resolved, that should fix the docs CI test.

Other than that, I'd add some more context to the unfreeze API docs. Since we'll still be supporting previously frozen indices (but not freezing new indices), I want to avoid confusion with the frozen data tier.

I'll take another look after that's addressed.

docs/reference/indices/apis/unfreeze.asciidoc Show resolved Hide resolved
docs/reference/indices/index-mgmt.asciidoc Outdated Show resolved Hide resolved
docs/reference/sql/language/indices.asciidoc Outdated Show resolved Hide resolved
danhermann and others added 7 commits September 8, 2021 12:41
Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
…sciidoc

Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
@danhermann
Copy link
Contributor Author

Thanks, @jrodewig. I resolved the merge conflict and incorporated your doc suggestions. I think there is still a problem with the docs build, but I'm not sure what it is.

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

Docs LGTM. I pushed one small change to relocate the deprecation admon, but everything else looks good. The docs CI should also be good now.

Thanks @danhermann!


<<freeze-index-api, Freezes>> an index.

deprecated[7.x,"The ILM Freeze action was deprecated in 7.x and will be treated as a no-op in 8.0+."]
Copy link
Contributor

@jrodewig jrodewig Sep 9, 2021

Choose a reason for hiding this comment

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

Didn't think of this until I saw a related email... should we leave these docs in until the ILM action is actually removed?

@danhermann
Copy link
Contributor Author

Didn't think of this until I saw a related email... should we leave these docs in until the ILM action is actually removed?

That's a good question. I'm not sure there's a lot of value in documenting a no-op action. What do you think @dakrone?

@dakrone
Copy link
Member

dakrone commented Sep 10, 2021

Didn't think of this until I saw a related email... should we leave these docs in until the ILM action is actually removed?

That's a good question. I'm not sure there's a lot of value in documenting a no-op action. What do you think @dakrone?

On one hand, it's nice to be able to see what something in a policy for the lifetime of 8.x does (since it won't be removed until 9.0). On the other hand, we have versioned API docs available, so I am okay to remove it also, since a user can go back to 7.x docs and see what it does.

So, I think it's okay to remove, since a user can go back to any 7.x docs and see what it does. Does that sound reasonable to you too @jrodewig?

@jrodewig
Copy link
Contributor

So, I think it's okay to remove, since a user can go back to any 7.x docs and see what it does. Does that sound reasonable to you too @jrodewig?

That seems reasonable to me. Thanks for the discussion, @danhermann @dakrone.

@danhermann
Copy link
Contributor Author

@elasticmachine update branch

@danhermann
Copy link
Contributor Author

Closed in favor of #78918

@danhermann danhermann closed this Oct 25, 2021
@danhermann danhermann deleted the 70192_remove_freeze_endpoint branch October 25, 2021 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Data Management/Indices APIs APIs to create and manage indices and templates Team:Clients Meta label for clients team Team:Data Management Meta label for data/management team v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants