Skip to content

Conversation

@williamrandolph
Copy link
Contributor

@williamrandolph williamrandolph commented Mar 12, 2020

The REST API uses "thread_pool" as the name of the thread pool metric. If we use this name internally when we serialize nodes stats and info requests, we won't need to do any fancy logic to check for and switch out "threadPool", which was the previous internal name.

I'm doing this in its own PR to isolate the amount of code for which I have to do a bwc-tests shuffle.

Relates #53410
Relates #52975

The REST API uses "thread_pool" as the name of the thread pool metric.
If we use this name internally when we serialize nodes stats and info
requests, we won't need to do any fancy logic to check for and switch
out "threadPool", which was the previous internal name.
@elasticmachine
Copy link
Collaborator

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

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Do we only need a constant change because this was accidentally modified in making the request contain a list of metrics?

@williamrandolph williamrandolph changed the title Use camel case for nodes stats/info metric names Use snake case for nodes stats/info metric names Mar 12, 2020
@williamrandolph
Copy link
Contributor Author

@rjernst I wouldn't say anything was accidentally modified. Rather, I picked a list of strings to use for request serialization, and used the old method names to make the strings. In particular, request.threadPool became "threadPool". I realized later on that it would be much more convenient for refactoring if I had used "thread_pool" instead. But at the moment the old getters and setters are mediating between the strings in the request and the strings used for serialization, so these constants don't have any wider effects.

In #53410 I'm trying to use these constants more widely and finding that I wish I had chosen them differently, but making the change breaks backwards compatibility with 7.x, so this PR is to make the change in isolation.

@williamrandolph
Copy link
Contributor Author

Another reason for making this change in isolation is that I want bwc tests to run cleanly on my next chunks of work.

@rjernst
Copy link
Member

rjernst commented Mar 12, 2020

These string values are what users are able to pass in via the url, like /stats/thread_pool right? I see our docs say thread_pool is the metric name, so this change seems to be inline with that, yet prior to this change it appears master/7.x would have expected threadPool, which would have been a breaking change. Can you verify this is the only metric that changed in the previous PR?

@williamrandolph
Copy link
Contributor Author

williamrandolph commented Mar 12, 2020

@rjernst No, these aren't the user-facing strings. That is the change I am planning to make in #53410. The current state of master and 7.x is that the RestNodesStatsRequest and RestNodesInfoRequest classes have their own separately-managed list of allowed metrics.

I just ran ./gradlew run on master and verified that curl localhost:9200/_nodes/thread_pool returns thread pool metrics and curl localhost:9200/_nodes/threadPool does not. Likewise for the nodes stats endpoint.

Another way to think of this is that NodesInfoRequest.Metric and NodesStatsRequest.Metric are, at the moment, only used for serializing and deserializing their respective Request objects, and this PR only changes one of these serialization/deserialization tokens so that it happens to align with the metric names used for the Rest API.

Here's the code that takes a metric name from the Rest request and sets the NodesInfoRequest:

nodesInfoRequest.threadPool(metrics.contains("thread_pool"));

And for Nodes Stats:

So you can see that the Metric enums aren't currently being used to translate between REST path parameters and Node(Stats|Info)Request objects.

As we discussed, the validation for these requests will probably move back to NodeService later in this sequence of refactorings, so I'm trying to keep things as simple as possible in the intermediate steps.

Copy link
Member

@rjernst rjernst 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 the explanation. LGTM.

@williamrandolph williamrandolph merged commit 7a18294 into elastic:master Mar 13, 2020
williamrandolph added a commit to williamrandolph/elasticsearch that referenced this pull request Mar 13, 2020
The REST API uses "thread_pool" as the name of the thread pool metric.
If we use this name internally when we serialize nodes stats and info
requests, we won't need to do any fancy logic to check for and switch
out "threadPool", which was the previous internal name.
williamrandolph added a commit that referenced this pull request Mar 13, 2020
* Use snake case for nodes stats/info metric names (#53446)

The REST API uses "thread_pool" as the name of the thread pool metric.
If we use this name internally when we serialize nodes stats and info
requests, we won't need to do any fancy logic to check for and switch
out "threadPool", which was the previous internal name.
@williamrandolph
Copy link
Contributor Author

Backported: #53535

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.

4 participants