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 & remove undocumented alternatives to the nodes hot threads API #52640

Closed
DaveCTurner opened this issue Feb 21, 2020 · 19 comments
Closed
Labels
:Core/Infra/REST API REST infrastructure and utilities good first issue low hanging fruit help wanted adoptme

Comments

@DaveCTurner
Copy link
Contributor

The nodes hot threads API has some little-known synonyms:

@Override
public List<Route> routes() {
return List.of(
new Route(GET, "/_cluster/nodes/hotthreads"),
new Route(GET, "/_cluster/nodes/hot_threads"),
new Route(GET, "/_cluster/nodes/{nodeId}/hotthreads"),
new Route(GET, "/_cluster/nodes/{nodeId}/hot_threads"),
new Route(GET, "/_nodes/hotthreads"),
new Route(GET, "/_nodes/hot_threads"),
new Route(GET, "/_nodes/{nodeId}/hotthreads"),
new Route(GET, "/_nodes/{nodeId}/hot_threads"));
}

These date back to its introduction in v0.20 but I do not think they have ever been documented. They seem unnecessary and I think we should remove them. There is of course a risk that someone out there is using one of these undocumented endpoints (perhaps due to a typo) and this is a breaking change so we should deprecate them first of course.

@DaveCTurner DaveCTurner added the :Core/Infra/REST API REST infrastructure and utilities label Feb 21, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/REST API)

@DaveCTurner DaveCTurner added good first issue low hanging fruit help wanted adoptme labels Feb 21, 2020
@muachilin
Copy link
Contributor

I would like to work on this! Thanks!

@DaveCTurner
Copy link
Contributor Author

Thanks @muachilin, go ahead. Note that this'll take two PRs in sequence: the first to deprecate these endpoints and the second to remove them, and both should be opened against the master branch. We will take care of backporting the first. See e.g. #50499 for an example of a PR that deprecates things in the REST API, and note particularly that this includes a test showing that we do indeed log the right warnings when the deprecated API is used.

@atmask
Copy link

atmask commented Feb 27, 2020

Hi, @DaveCTurner I was looking to help on this issue but it looks as though the endpoints are already deprecated?
https://github.com/elastic/elasticsearch/blob/master/rest-api-spec/src/main/resources/rest-api-spec/api/nodes.hot_threads.json#L104

@muachilin
Copy link
Contributor

muachilin commented Feb 27, 2020

I am already working on this to add tests and deprecate warnings for this file!

@muachilin
Copy link
Contributor

@atmask Do you want to work on this? But I've claimed this issue

@atmask
Copy link

atmask commented Feb 28, 2020

@muachilin Ah I see, I didn't realize the issue was actively being taken care of! All yours!

@muachilin
Copy link
Contributor

@atmask Thank you very much!

@DaveCTurner
Copy link
Contributor Author

DaveCTurner commented Feb 28, 2020

Interesting observation @atmask, thanks. I had indeed not noticed that these endpoints were documented and deprecated, in the REST API spec (see #39063). We also need the deprecation warnings in the REST handler too since there's a risk that clients won't all notice them. For instance, as noted in #39063 (comment), Kibana's Console client ignores deprecations in the REST API spec but does respect deprecation warnings emitted by the handlers themselves.

@muachilin
Copy link
Contributor

muachilin commented Feb 28, 2020

@DaveCTurner I see! I have added warnings and made some tests about this. The related PR is #52930.

@jasontedor
Copy link
Member

This was deprecated in 7.7.0 in #52930. @muachilin are you interested in proceeding with the removal of the deprecated endpoints in the master branch?

@muachilin
Copy link
Contributor

@jasontedor Sure!

@muachilin
Copy link
Contributor

Thank you :)

@mathewTH
Copy link

mathewTH commented Apr 6, 2020

Is this still being worked on?

@jasontedor
Copy link
Member

@muachilin Did you still want to work on removing the dedicated endpoints from master (8.0.0)?

@erickmp07
Copy link

@jasontedor If he didn't, I'd like to work on it.

@jasontedor
Copy link
Member

@erickmp07 Thanks. Let's give @muachilin some time to reply. If there's no reply by this time tomorrow, it's yours.

@muachilin
Copy link
Contributor

Hi! @erickmp07 @jasontedor I would like to work on it! Thank you :)

@muachilin
Copy link
Contributor

@jasontedor I open a PR (#55109) to remove the deprecated end points for hot threads API. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities good first issue low hanging fruit help wanted adoptme
Projects
None yet
Development

No branches or pull requests

7 participants