Create set-based interface for NodesInfoRequest #53410
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit begins the work of removing the "hard-coded" metric getters and setters from the NodesInfoRequest classes. We start by providing new flexible getters and setters. We then update the test classes to remove the old getters, and then remove those getters.
Throw an IllegalStateException if an unknown metric is added or removed. Replace some of the infrequently used setters.
Based on some of the refactoring I'm doing, it seems like it's convenient to be have an addMetrics(String ...) method. Also add some more test coverage.
williamrandolph
added
>non-issue
:Core/Infra/Plugins
|
Pinging @elastic/es-core-infra (:Core/Infra/Plugins) |
williamrandolph
commented
Mar 11, 2020
| * @param metricName Name of the metric to include or remove. | ||
| */ | ||
| private void addOrRemoveMetric(boolean includeMetric, String metricName) { | ||
| if (includeMetric) { | ||
| private void optionallyAddMetric(boolean addMetric, String metricName) { |
There was a problem hiding this comment.
This method is now only used for backwards-compatible deserialization, and doesn't need to be able to remove metrics anymore.
|
@elasticmachine run elasticsearch-ci/bwc |
williamrandolph
commented
Mar 12, 2020
server/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoRequest.java
Outdated
Show resolved
Hide resolved
rjernst
reviewed
Mar 12, 2020
server/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoRequest.java
Show resolved
Hide resolved
| nodesInfoRequest.plugins(metrics.contains("plugins")); | ||
| nodesInfoRequest.ingest(metrics.contains("ingest")); | ||
| nodesInfoRequest.indices(metrics.contains("indices")); | ||
| // disregard unknown metrics |
There was a problem hiding this comment.
Aren't unknown metrics an error case?
There was a problem hiding this comment.
The current behavior of the Nodes Info API is to silently disregard invalid metrics (or node ids). The Nodes Stats endpoint, on the other hand, will return an error if you request an unknown metric.
Arguably, the Nodes Info API should return an error for unknown metrics, but that would go beyond just refactoring.
rjernst
reviewed
Mar 13, 2020
server/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoRequest.java
Outdated
Show resolved
Hide resolved
williamrandolph
commented
Mar 16, 2020
server/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoRequest.java
Outdated
Show resolved
Hide resolved
rjernst
approved these changes
Mar 24, 2020
server/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoRequest.java
Outdated
Show resolved
Hide resolved
williamrandolph
added a commit
to williamrandolph/elasticsearch
that referenced
this pull request
Mar 25, 2020
This commit begins the work of removing the "hard-coded" metric getters and setters from the NodesInfoRequest classes. We start by providing new flexible getters and setters. We then update the test classes to remove the old getters, and then remove those getters.
williamrandolph
added a commit
that referenced
this pull request
Mar 26, 2020
This commit begins the work of removing the "hard-coded" metric getters and setters from the NodesInfoRequest classes. We start by providing new flexible getters and setters. We then update the test classes to remove the old getters, and then remove those getters.
|
Backported: #54223 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is another refactoring PR to move the Nodes Information endpoint in the direction of pluggability (see #52975). In #53140, we changed the
NodesInfoRequestclass so that it serializes as a set of strings rather than a fixed list of booleans. In this PR, we update the interface ofNodesInfoRequestso that it uses getters and setters that are more appropriate to this internal structure. For example, the old way of adding "os" metrics to a request would be to callrequest.os(true). The new way of doing this is to callrequest.addMetric("os"). For the time being, theNodesInfoRequesthas aMetricenum that serves as the source of truth for allowed metrics. I plan to replace this eventually with a pluggable solution.The follow-up step for this PR is to update the
NodesInfoRequestBuilderto have a matching set-based interface, but I am putting that off to another PR in order to keep the diff small and to avoid confusion between the two classes.