-
Notifications
You must be signed in to change notification settings - Fork 25.7k
cat API: added endpoint for Circuit Breakers #136890
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
Conversation
Added CAT Action to display Circuit Breakers stats for all nodes. The API supports pattern matching as a path parameter and the standard query parameters of CAT actions. Addresses elastic#132688
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @mamazzol, I've created a changelog YAML for you. |
ldematte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I left a couple of comments, more for discussion and learning really.
| @Override | ||
| public void processResponse(final ClusterStateResponse clusterStateResponse) { | ||
| NodesStatsRequest nodesStatsRequest = new NodesStatsRequest( | ||
| clusterStateResponse.getState().nodes().stream().map(DiscoveryNode::getId).toArray(String[]::new) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of a pity to have DiscoveryNode here, get the ID, and then have BaseNodeRequest#resolveNodes find the DiscoveryNode again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little subtle, but this should be unnecessary: AFAICT we're always getting these stats from all nodes, but that's the default behaviour, we can just pass the empty array. See org.elasticsearch.action.support.nodes.BaseNodesRequest#resolveNodes which calls org.elasticsearch.cluster.node.DiscoveryNodes#resolveNodes which does this:
elasticsearch/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java
Lines 438 to 439 in f3a1664
| if (nodes == null || nodes.length == 0) { | |
| return stream().map(DiscoveryNode::getId).toArray(String[]::new); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! After looking through the code I saw what you were pointing at, it is indeed redundant to get the nodes from the ClusterState with a separate request. I have removed it
| clusterStateResponse.getState().nodes().stream().map(DiscoveryNode::getId).toArray(String[]::new) | ||
| ); | ||
| nodesStatsRequest.clear().addMetric(NodesStatsRequestParameters.Metric.BREAKER); | ||
| client.admin().cluster().nodesStats(nodesStatsRequest, new RestResponseListener<>(channel) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to avoid this kind of nesting, and possibly generate better response in case of failure, you might consider ListenableFuture. It makes reasoning more complicated though (IMO). See RestShardsAction for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please prefer SubscribableListener over ListenableFuture. They're basically the same, except ListenableFuture adds multiple layers of wrapping around the exceptions it encounters for largely-historical reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip @DaveCTurner!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll keep that in mind for the future. For now, after removing the first request there's only 1 action executed so it shouldn't be needed.
|
Looking good, @mamazzol ! To skip the test when running backwards compatibility tests against older versions, you can use a capability (which is just the endpoint itself in this case, see https://github.com/elastic/elasticsearch/blob/main/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/README.asciidoc#require-or-skip-api-capabilities) For examples you can have a look at |
mosche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @mamazzol, great work.
Just a tiny nit before merging.
|
|
||
| @Override | ||
| public Set<String> supportedCapabilities() { | ||
| return Sets.union(Set.of("cat_circuit_breaker"), super.supportedCapabilities()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary, the pure existence of the REST endpoint should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know how I can verify it?
I saw other tests added this capability filter, like cat_circuit_breaker in my case, but where does that get defined as a capability, if not in that method? ( I trust you, I am just curious :D )
| capabilities: | ||
| - method: GET | ||
| path: /_cat/circuit_breaker | ||
| capabilities: [ cat_circuit_breaker ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| capabilities: [ cat_circuit_breaker ] |
As mentioned above, defining and checking for a capability shouldn't be needed here. Existence should do
|
🥳 |
Added CAT Action to display Circuit Breakers stats for all nodes. The API supports pattern matching as a path parameter and the standard query parameters of CAT actions. This change includes spec and yamlRestTest. Addresses elastic#132688
Added CAT Action to display Circuit Breakers stats for all nodes. The API supports pattern matching as a path parameter and the standard query parameters of CAT actions. This change includes spec and yamlRestTest. Addresses elastic#132688
Added CAT Action to display Circuit Breakers stats for all nodes. The API supports pattern matching as a path parameter and the standard query parameters of CAT actions (help, format, h...).
action.handleRequest(restRequest, channel, nodeClient);to retrieve the response from the channel from the main thread. The flow executed fromcontroller.dispatchRequestinRestActionTestCasechunks the response and the encoding is required to happen on a Transport thread.Addresses: #132688