Resurrect hot_threads routes #73855

joegallo · 2021-06-07T15:39:14Z

Related to #51816 / #68905. Also related to #52640 / #55109.

Adds back the various removed hot_threads routes when using rest compatibility for a request.

joegallo · 2021-06-07T15:46:00Z

...amlRestCompatTest/resources/rest-api-spec/test/v7compat/nodes.hotthreads/10_basic_compat.yml

+      warnings_regex:
+        - ".*hot_?threads.* is a deprecated endpoint.*"
+  - match:
+      $body: /:::/


This match is not my most favorite code ever. I wanted to verify that the response actually resembles a hot_threads response, but the regex match has to be on the first line and there's not all that much for us to match on there:

::: {runTask-0}{rNO3-C-BStGLmQ_xUdIecA}{PiMCJKfOSnuRkp0Uk1E7ig}{127.0.0.1}{127.0.0.1:9300}{cdfhilmrstw}{ml.machine_memory=68719476736, xpack.installed=true, testattr=test, ml.max_open_jobs=512, ml.max_jvm_size=536870912} Hot threads at 2021-06-07T15:44:46.079Z, interval=500ms, busiestThreads=3, ignoreIdleThreads=true:

Anyway, there is a ::: so I'm matching on that. A reasonable alternative would be to cut the match entirely and just run with "any non-error response is good enough".

joegallo · 2021-06-07T15:48:17Z

One point of discussion:

joegallo@galactic:~/Code/elastic/elasticsearch $ curl -s -XGET 'http://localhost:9200/_cluster/nodes/hot_threads'
{"error":"no handler found for uri [/_cluster/nodes/hot_threads] and method [GET]"}%
joegallo@galactic:~/Code/elastic/elasticsearch $ curl -s -XGET -H'accept:application/json;compatible-with=7' 'http://localhost:9200/_cluster/nodes/hot_threads'
::: {runTask-0}{rNO3-C-BStGLmQ_xUdIecA}{PiMCJKfOSnuRkp0Uk1E7ig}{127.0.0.1}{127.0.0.1:9300}{cdfhilmrstw}{ml.machine_memory=68719476736, xpack.installed=true, testattr=test, ml.max_open_jobs=512, ml.max_jvm_size=536870912}
   Hot threads at 2021-06-07T15:44:46.079Z, interval=500ms, busiestThreads=3, ignoreIdleThreads=true:

joegallo@galactic:~/Code/elastic/elasticsearch $ curl -s -XGET -H'accept:text/plain;compatible-with=7' 'http://localhost:9200/_cluster/nodes/hot_threads'
::: {runTask-0}{rNO3-C-BStGLmQ_xUdIecA}{PiMCJKfOSnuRkp0Uk1E7ig}{127.0.0.1}{127.0.0.1:9300}{cdfhilmrstw}{ml.machine_memory=68719476736, xpack.installed=true, testattr=test, ml.max_open_jobs=512, ml.max_jvm_size=536870912}
   Hot threads at 2021-06-07T15:47:12.956Z, interval=500ms, busiestThreads=3, ignoreIdleThreads=true:

Given that the response is not JSON -- should application/json work here? Is text/plain intended to be supported in this way? I'm not 100% sure I'm holding it right.

DaveCTurner · 2021-06-07T16:29:25Z

It's legitimate, although surprising, according to RFC7231 section 5.3.2:

                                            If the header field is
   present in a request and none of the available representations for
   the response have a media type that is listed as acceptable, the
   origin server can either honor the header field by sending a 406 (Not
   Acceptable) response or disregard the header field by treating the
   response as if it is not subject to content negotiation.

jakelandis · 2021-06-07T17:01:14Z

rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/api/nodes.hotthreads.json

+    "url":{
+      "paths":[
+        {
+          "path":"/_cluster/nodes/hotthreads",


would it more correct to re-introduce this path (and the like) as deprecated in the spec for 7.x and add the allowed warning (for any tests that call this API) ?

This would allow you to avoid introducing the spec only for compatibility.

I like that idea! Just to make sure I'm following -- that'd be a PR pointed at 7.x (no version that hits master) that adds these as deprecated endpoints to the rest-api-spec, and then in this PR we wouldn't need a new test or any specs, because this would all execute automatically by way of the yamlRestCompatTest tests.

Is there any downside to adding these as deprecated in 7.x? I'm thinking, for example, that perhaps the autogenerated client code might actually start using them (if so, that'd be strictly worse, and we probably wouldn't want to go this way).

@pgomulka what do you think?

i like that idea too!
if a test client will use it, maybe we can ignore the warning in 7.x for this? Like we do for types removal in 7.x

that'd be a PR pointed at 7.x (no version that hits master) that adds these as deprecated endpoints to the rest-api-spec,

yes, that is what i meant. I think it works well in this case (and any other one off cases) since it probably should have been that way from the start and should have minimal pollution in 7.x.

However, I don't think this should be a rule of thumb for all missing paths. For example, for the _xpack/ prefix i can see the argument for not polluting 7.x since there are so many, somewhat inconsistent (depending on when it was added), and has a lot of associated tests that would need to allow warnings.

elasticmachine · 2021-06-07T17:57:10Z

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

swallez · 2021-06-07T20:42:05Z

After a discussion with @joegallo about route deprecations and how clients handle them: a client selects the route that matches the parameters specified by the application (e.g. non null in Java). What I'm not sure about is what happens when there are two matching routes. We have to make sure deprecated routes have lower priority in the selection algorithm and that we don't just choose the first matching route. I'll bring this as a discussion/verification topic to the clients team.

pgomulka

LGTM

pgomulka · 2021-06-08T06:51:15Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesHotThreadsAction.java


 import static org.elasticsearch.rest.RestRequest.Method.GET;

 public class RestNodesHotThreadsAction extends BaseRestHandler {

+    private static final String formatDeprecatedMessageWithoutNodeID = "[%s] is a deprecated endpoint. " +


just a thought. we could just use one message for all. We have a used endpoint as a key, API_GET_/_nodes/{nodeid}/hot_threads
not as readable as here, but might save some effort
but it looks good now too!

I basically did a revert + edit of the commit that removed the apis, so it was cheap to do it as is (and keeps the code mostly in line with the way it is on 7.x).

pgomulka · 2021-06-08T06:52:19Z

rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/api/nodes.hotthreads.json

+    "url":{
+      "paths":[
+        {
+          "path":"/_cluster/nodes/hotthreads",


i like that idea too!
if a test client will use it, maybe we can ignore the warning in 7.x for this? Like we do for types removal in 7.x

pgomulka · 2021-06-08T06:55:38Z

re text/plain;compatible-with=7 this possibly works by mistake at the moment and is a flavour of this #72969

We can rely on the one from 7.x, which already includes the deprecated routes. But since it includes both the deprecated routes and the non-deprecated routes, we need to switch the warnings_regex to an allowed_warnings_regex.

joegallo · 2021-06-08T14:26:54Z

joegallo@galactic:~/Code/elastic/elasticsearch-7.x $ cat rest-api-spec/src/main/resources/rest-api-spec/api/nodes.hot_threads.json | grep -E 'path|deprecated'
      "paths":[
          "path":"/_nodes/hot_threads",
          "path":"/_nodes/{node_id}/hot_threads",
          "path":"/_cluster/nodes/hotthreads",
          "deprecated":{
          "path":"/_cluster/nodes/{node_id}/hotthreads",
          "deprecated":{
          "path":"/_nodes/hotthreads",
          "deprecated":{
          "path":"/_nodes/{node_id}/hotthreads",
          "deprecated":{
          "path":"/_cluster/nodes/hot_threads",
          "deprecated":{
          "path":"/_cluster/nodes/{node_id}/hot_threads",
          "deprecated":{

Some enterprising developer liked this conversation so much they went back in time and made 7.x already have those routes in there as deprecated. So I've dropped my copy from this PR and switched the warnings_regex to an allowed_warnings_regex (sometimes the test will hit the non-deprecated APIs, so we can't rely on the warning appearing).

jakelandis

LGTM

joegallo added 2 commits June 7, 2021 11:36

Resurrect the removed hot threads endpoints

cf2a563

Add a simple yamlRestCompatTest test

05e3d84

joegallo added >non-issue v8.0.0 labels Jun 7, 2021

joegallo requested review from jakelandis and pgomulka June 7, 2021 15:39

joegallo commented Jun 7, 2021

View reviewed changes

Merge branch 'master' into resurrect-hotthreads

7d63126

jakelandis reviewed Jun 7, 2021

View reviewed changes

sethmlarson added the Team:Clients Meta label for clients team label Jun 7, 2021

pgomulka approved these changes Jun 8, 2021

View reviewed changes

joegallo added 2 commits June 8, 2021 10:02

Drop the additional rest-api-spec here

2c01275

We can rely on the one from 7.x, which already includes the deprecated routes. But since it includes both the deprecated routes and the non-deprecated routes, we need to switch the warnings_regex to an allowed_warnings_regex.

Merge branch 'master' into resurrect-hotthreads

c487c7e

joegallo requested a review from jakelandis June 8, 2021 14:27

Merge branch 'master' into resurrect-hotthreads

8c5229d

jakelandis approved these changes Jun 8, 2021

View reviewed changes

joegallo merged commit 4bb1f28 into elastic:master Jun 8, 2021

joegallo deleted the resurrect-hotthreads branch June 8, 2021 20:19

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resurrect hot_threads routes #73855

Resurrect hot_threads routes #73855

joegallo commented Jun 7, 2021 •

edited

Loading

joegallo Jun 7, 2021

joegallo Jun 7, 2021 •

edited

Loading

joegallo commented Jun 7, 2021

DaveCTurner commented Jun 7, 2021

jakelandis Jun 7, 2021

joegallo Jun 7, 2021

pgomulka Jun 8, 2021

jakelandis Jun 8, 2021

elasticmachine commented Jun 7, 2021

swallez commented Jun 7, 2021 •

edited

Loading

pgomulka left a comment

pgomulka Jun 8, 2021

joegallo Jun 8, 2021

pgomulka Jun 8, 2021

pgomulka commented Jun 8, 2021

joegallo commented Jun 8, 2021

jakelandis left a comment

Resurrect hot_threads routes #73855

Resurrect hot_threads routes #73855

Conversation

joegallo commented Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

joegallo Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

joegallo commented Jun 7, 2021

DaveCTurner commented Jun 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 7, 2021

swallez commented Jun 7, 2021 • edited Loading

pgomulka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka commented Jun 8, 2021

joegallo commented Jun 8, 2021

jakelandis left a comment

Choose a reason for hiding this comment

joegallo commented Jun 7, 2021 •

edited

Loading

joegallo Jun 7, 2021 •

edited

Loading

swallez commented Jun 7, 2021 •

edited

Loading