Skip to content

Conversation

@boicehuang
Copy link
Contributor

The cat shards 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 15:00
@danhermann
Copy link
Contributor

@elasticmachine ok to test

@elastic elastic deleted a comment from elasticmachine Sep 22, 2020
@elastic elastic deleted a comment from elasticmachine Sep 22, 2020
@danhermann
Copy link
Contributor

@elasticmachine run elasticsearch-ci/docs

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, this looks pretty good, too. There are a couple simple changes necessary below that are similar to the ones on your other PR.

Comment on lines 276 to 284
include::{es-repo-dir}/rest-api/common-parms.asciidoc[tag=local]
+
--
`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.

Suggested change
include::{es-repo-dir}/rest-api/common-parms.asciidoc[tag=local]
+
--
`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.]
--
`local`::
(Optional, boolean)
+
deprecated::[8.0.0,"This parameter does not affect the request. It will be removed in a future version."]

Comment on lines 67 to 68
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.";

@boicehuang boicehuang requested a review from danhermann October 16, 2020 10:13
@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.

@boicehuang
Copy link
Contributor Author

boicehuang commented Oct 20, 2020

@danhermann i can't find the "Allow edits and access to secrets by maintainers" option on the PR.

image

but I have merged the master branch into it. Can you merge it? The same on #62198

@danhermann
Copy link
Contributor

@danhermann i can't find the "Allow edits and access to secrets by maintainers" option on the PR.
but I have merged the master branch into it. Can you merge it? The same on #62198

That might be because the PR originates from the TencentCloudES repository rather than your personal fork of the Elasticsearch repository. That will just mean that we can't use our automated tooling to merge in master.

In order to get this PR merged, we need all the tests to pass. Right now, one test is failing due to:

[ant:checkstyle] [ERROR] /dev/shm/elastic+elasticsearch+pull-request-1/server/src/main/java/org/elasticsearch/rest/action/cat/RestShardsAction.java:23:8: Unused import - org.elasticsearch.Version

which should be an easy fix. A second test is failing for what is probably an unrelated reason, but they will be re-run after you remove the unused import above.

@boicehuang
Copy link
Contributor Author

@danhermann I have fixed it.

@danhermann
Copy link
Contributor

Thanks, @boicehuang. Almost everything looks good on this. I apologize for not catching this earlier, but the documentation line here:

https://github.com/elastic/elasticsearch/pull/62197/files#diff-d3ff1cd557d7671bd30a0a1fb5a4162e40e8a19c7a7528c336f32c822d178c93R279

should read:

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

because we're already past the point at which code changes are going into the 7.10 release. We'll make sure that documentation is correctly updated to the appropriate 7.x release before we get to the 7.11 release.

@danhermann
Copy link
Contributor

@boicehuang, note that it is the line the shards.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 7d65278 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.

5 participants