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

Deprecate the 'local' parameter of /_cat/indices #62198

Merged
merged 7 commits into from
Oct 29, 2020

Conversation

boicehuang
Copy link
Contributor

The cat indices APIs perform a ClusterStateAction then an IndicesStatsAction. They accept the ?local parameter and pass this to the ClusterStateAction but this parameter has no effect on the IndicesStatsAction.

This commit deprecates the ?local parameter on this API so that it can be removed in 8.0.

Related #60718

@matriv matriv added the :Data Management/CAT APIs Text APIs behind /_cat label Sep 10, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/CAT APIs)

@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Sep 10, 2020
@danhermann danhermann self-requested a review September 10, 2020 14:59
@danhermann
Copy link
Contributor

@elasticmachine ok to test

@danhermann
Copy link
Contributor

@elasticmachine update branch

@elasticmachine
Copy link
Collaborator

user doesn't have permission to update head repository

Copy link
Contributor

@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.

@boicehuang, thanks for addressing this issue. I think we can get this merged with a couple minor changes as noted below.

Comment on lines 78 to 85
+
--
`local`::
(Optional, boolean) If true, the request retrieves information from the local node
only. Defaults to false, which means information is retrieved from the master node.
deprecated::[8.0,This parameter does not cause this API to act locally. It will be
removed in version 8.0.]
--
Copy link
Contributor

Choose a reason for hiding this comment

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

This creates two entries for the local parameter, one pulled from the common-parms file and this one. I'll solicit some input from our documentation writers on how best to handle this.

Copy link
Contributor

@danhermann danhermann Sep 22, 2020

Choose a reason for hiding this comment

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

@jrodewig, do you have any suggestions on how to handle this documentation case where a parameter pulled in from a common file needs to be marked as deprecated on a specific API?

Copy link
Contributor

Choose a reason for hiding this comment

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

I left a suggestion here: #62198 (comment)

Comment on lines 71 to 72
static final String LOCAL_DEPRECATED_MESSAGE = "Deprecated parameter [local] used. This parameter does not cause this API to act " +
"locally, and should not be used. It will be unsupported in version 8.0.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static final String LOCAL_DEPRECATED_MESSAGE = "Deprecated parameter [local] used. This parameter does not cause this API to act " +
"locally, and should not be used. It will be unsupported in version 8.0.";
static final String LOCAL_DEPRECATED_MESSAGE = "The parameter [local] is deprecated and will be removed in a future release.";

"description":"Return local information, do not retrieve the state from master node (default: false)"
"description":"Return local information, do not retrieve the state from master node (default: false)",
"deprecated":{
"version":"8.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be the release in which we deprecate the parameter? That should be before 8.0.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

We typically update the version when backporting to a release branch and then come back and update the master branch.

docs/reference/cat/indices.asciidoc Outdated Show resolved Hide resolved
@elastic elastic deleted a comment from elasticmachine Sep 23, 2020
@boicehuang boicehuang requested a review from danhermann October 16, 2020 10:12
@jrodewig
Copy link
Contributor

@elasticmachine update branch

@elasticmachine
Copy link
Collaborator

user doesn't have permission to update head repository

@jrodewig
Copy link
Contributor

@boicehuang Will you merge the latest changes from elastic/master into this branch? There are several CI tests which have failed. We'll need to get those tests to pass before merging this PR. Thanks!

@danhermann
Copy link
Contributor

@boicehuang, could you enable the "Allow edits and access to secrets by maintainers" option on this PR? That will enable us to use some of our automated tooling to do things like merge in the master branch, etc. I'll re-review it shortly. From a quick glance, it looks like we'll be able to merge it.

@boicehuang
Copy link
Contributor Author

@danhermann ok

@danhermann
Copy link
Contributor

@elasticmachine update branch

@elasticmachine
Copy link
Collaborator

user doesn't have permission to update head repository

@boicehuang
Copy link
Contributor Author

@danhermann sorry, try again

@jrodewig
Copy link
Contributor

@elasticmachine update branch

@elastic elastic deleted a comment from elasticmachine Oct 18, 2020
@danhermann
Copy link
Contributor

@boicehuang, this is ready to merge with the one exception of the documentation line here:

https://github.com/elastic/elasticsearch/pull/62198/files#diff-3c6feaa818c49894e13b5d59e34607fa68f4f0d4002012cb2400e7ddf79dcc58R80

which should be:

deprecated::[8.0.0,"This parameter does not affect the request. It will be removed in a future release."]

@danhermann
Copy link
Contributor

@boicehuang, note that it is the line the indices.asciidoc file that needs to be updated. Also, please merge master into this PR so that the tests will pass. Normally, one of us would do that for you, but we cannot on this PR since edits aren't allowed.

@danhermann danhermann merged commit 41fbc52 into elastic:master Oct 29, 2020
@danhermann
Copy link
Contributor

Thanks, @boicehuang. I will get this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants