Prune unnecessary information from TransportNodesInfoAction.NodeInfoRequest #99938

NEUpanning · 2023-09-27T07:39:27Z

Today every node-level TransportNodesInfoAction.NodeInfoRequest carries the entire top-level NodesInfoRequest, but it only needs to carry the metrics. The uneccessary information like nodes list incurs extra effort and network traffic.

This commit trims these things down. There are certainly other places (e.g. ,TransportNodesInfoAction.NodeStatsRequest ) where this pruning request would be valuable, but here we only focus on the TransportNodesInfoAction.NodeInfoRequest for the performance optimization of nodes info action.

Relates #99744

elasticsearchmachine · 2023-09-27T07:53:32Z

Pinging @elastic/es-data-management (Team:Data Management)

DaveCTurner · 2023-09-27T07:54:04Z

As it stands this doesn't pass ./gradlew precommit, and it fails the BWC test suite too (e.g. ./gradlew bwctest)

NEUpanning · 2023-09-27T08:01:17Z

Got it.I will make sure my code passes ./gradlew precommit.

DaveCTurner · 2023-09-27T08:01:21Z

I like that we extract NodeInfoMetrics to a top-level class here, but that does make it hard to see the actual change in behaviour introduced by this PR. Could we do that refactoring first in a separate PR which doesn't change any behaviour and then follow up with the optimization?

NEUpanning · 2023-09-27T08:07:13Z

Sure, i will do that refactoring first in a separate PR and then open this PR for the optimization.

NEUpanning · 2023-09-28T02:17:34Z

I do that refactoring in a this PR (#99990 ). Could you take a look?

NEUpanning · 2023-09-28T09:46:30Z

I have updated commit. Could you please take a look?

DaveCTurner · 2023-09-28T10:29:48Z

Much better :) But it still fails the BwC test suite as I mentioned above.

NEUpanning · 2023-09-28T10:45:38Z

Sorry, i forgot about that.I will make sure my code passes ./gradlew bwctest.

NEUpanning · 2023-09-29T03:08:12Z

I can't even pass ./gradlew bwctest locally on the main branch. Errors like this
Caused by: java.lang.RuntimeException: Timed out after PT2M waiting for ports files for: { cluster: 'test-cluster', node: 'test-cluster-1' }
I pushed commit about backwards compatibility. Could we let elasticsearchmachine test this?

DaveCTurner · 2023-09-29T07:07:08Z

...src/main/java/org/elasticsearch/action/admin/cluster/node/info/TransportNodesInfoAction.java

+            if (out.getTransportVersion().onOrAfter(NODE_INFO_REQUEST_VERSIONS_ADDED)) {
+                this.nodesInfoMetrics.writeTo(out);
+            } else {
+                this.request.writeTo(out);


I think it would work to create a new NodesInfoRequest right here just for sending out to older nodes (with no node IDs, just the metrics). That way we wouldn't have to keep track of the request and the metrics separately, because that's going to be problematic if we ever tried to send one of these things over the wire twice with two different transport versions.

Big thanks, such a great idea! This way we don't have to worry about backwards compatibility. I have force pushed commits to make them easier to review. fa7c890

…uest

DaveCTurner

Ah sorry - this looks good for the BwC path but it's kind of a workaround and we'd want to remove it as soon as possible (i.e. in v9.0). For future versions we should be sending only a NodesInfoMetrics as per your original commit.

Also please don't force-push to branches once they're being reviewed - as a reviewer it makes it much harder to keep track of what I have/haven't seen.

DaveCTurner · 2023-10-01T14:31:33Z

I pushed 7ccf073 adding a changelog entry for this PR - make sure to include that in future commits too please.

NEUpanning · 2023-10-01T14:54:40Z

Got it. In the future i will add a changelog and not force-push.

NEUpanning · 2023-10-01T15:02:24Z

For future versions i would like to do this work, at that time you can mention me.

Ah sorry - this looks good for the BwC path but it's kind of a workaround and we'd want to remove it as soon as possible (i.e. in v9.0). For future versions we should be sending only a NodesInfoMetrics as per your original commit.

DaveCTurner · 2023-10-01T15:15:04Z

For future versions i would like to do this work, at that time you can mention me.

Thanks for the offer! If we get this PR right then preparing for the 9.0 release will just be a case of removing the checks on transport versions, which is easiest done across the whole codebase in one go so, to set expectations, I doubt we will ask for help with that.

NEUpanning · 2023-10-01T16:15:11Z

I think i finally understand what you mean. We only track NodesInfoMetrics, for older nodes a NodesInfoMetrics is sent, for newer nodes a new NodesInfoRequest (with no node IDs) is sent and for future versions removing the operation of the older nodes.

DaveCTurner · 2023-10-01T16:27:14Z

for older nodes a NodesInfoMetrics is sent, for newer nodes a new NodesInfoRequest (with no node IDs) is sent

I think you meant the other way around: to current/future versions we want to send a bare NodesInfoMetrics but older nodes require us to send a complete NodesInfoRequest (which we can just synthesize here if needed, omitting all the node IDs)

…ut older nodes require us to send a complete NodesInfoRequest

NEUpanning · 2023-10-02T07:58:29Z

Yes, that's a clerical error. I pushed b058d43 .

DaveCTurner

Looks great, one naming nit/suggestion and then I expect this is ready to go.

server/src/main/java/org/elasticsearch/TransportVersions.java

transport version naming Co-authored-by: David Turner <david.turner@elastic.co>

DaveCTurner

LGTM

DaveCTurner · 2023-10-02T11:11:41Z

@elasticmachine test this please

NEUpanning · 2023-10-02T12:09:51Z

Thanks for your review David :)

DaveCTurner · 2023-10-02T12:18:14Z

You're welcome @NEUpanning , thanks for your contribution!

NEUpanning · 2023-10-02T12:35:33Z

It's my pleasure and I learned a lot.

…equest (elastic#99938) There's no need to include the whole top-level `NodesInfoRequest` in the requests for info from individual nodes, and this can add substantial overhead if there are lots of nodes in the cluster. With this commit we drop the wrapper in favour of just the parts of the top-level request needed for the node-level processing. Relates elastic#99744

NEUpanning · 2023-10-07T02:40:23Z

I would like to make this change to TransportNodesStatsAction.NodeStatsRequest in the same way, do you think I should do it?

DaveCTurner · 2023-10-07T07:36:25Z

I would like to make this change to TransportNodesStatsAction.NodeStatsRequest in the same way, do you think I should do it?

Sounds good to me, yes. I think the same approach with 2 PRs will work fine there too.

DaveCTurner · 2023-10-16T06:03:40Z

FWIW we have the same inefficiency in several other places too, I opened #100878 to list them all.

NEUpanning · 2023-10-16T07:56:10Z

I would like to make this change to TransportNodesStatsAction.NodeStatsRequest in the same way

I did the first step, refatoring NodesStatsRequest in #100883

elasticsearchmachine added v8.11.0 needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team labels Sep 27, 2023

NEUpanning mentioned this pull request Sep 27, 2023

Improve performance of Cat Nodes API #99744

Closed

DaveCTurner added >enhancement :Data Management/Stats Statistics tracking and retrieval APIs and removed needs:triage Requires assignment of a team area label labels Sep 27, 2023

elasticsearchmachine added the Team:Data Management Meta label for data/management team label Sep 27, 2023

DaveCTurner self-assigned this Sep 27, 2023

NEUpanning mentioned this pull request Sep 28, 2023

Refactor NodesInfoRequest to extract NodeInfoMetrics to a top-level class from it #99990

Merged

NEUpanning force-pushed the prune_node_info_request branch from 44499e2 to 511c984 Compare September 28, 2023 09:41

DaveCTurner reviewed Sep 29, 2023

View reviewed changes

Prune unnecessary information of TransportNodesInfoAction#NodeInfoReq…

fa7c890

…uest

NEUpanning force-pushed the prune_node_info_request branch from f36f302 to fa7c890 Compare October 1, 2023 00:35

DaveCTurner reviewed Oct 1, 2023

View reviewed changes

Add changelog

7ccf073

DaveCTurner changed the title ~~Prune unnecessary information of TransportNodesInfoAction.NodeInfoReqest~~ Prune unnecessary information from TransportNodesInfoAction.NodeInfoRequest Oct 1, 2023

to current/future versions we want to send a bare NodesInfoMetrics b…

b058d43

…ut older nodes require us to send a complete NodesInfoRequest

Merge branch 'main' into prune_node_info_request

86a5a56

DaveCTurner reviewed Oct 2, 2023

View reviewed changes

server/src/main/java/org/elasticsearch/TransportVersions.java Outdated Show resolved Hide resolved

NEUpanning and others added 2 commits October 2, 2023 18:58

Update server/src/main/java/org/elasticsearch/TransportVersions.java

db48fab

transport version naming Co-authored-by: David Turner <david.turner@elastic.co>

naming TransportVersions

7d02dbf

DaveCTurner approved these changes Oct 2, 2023

View reviewed changes

DaveCTurner merged commit df52701 into elastic:main Oct 2, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prune unnecessary information from TransportNodesInfoAction.NodeInfoRequest #99938

Prune unnecessary information from TransportNodesInfoAction.NodeInfoRequest #99938

NEUpanning commented Sep 27, 2023 •

edited

elasticsearchmachine commented Sep 27, 2023

DaveCTurner commented Sep 27, 2023

NEUpanning commented Sep 27, 2023

DaveCTurner commented Sep 27, 2023

NEUpanning commented Sep 27, 2023

NEUpanning commented Sep 28, 2023

NEUpanning commented Sep 28, 2023

DaveCTurner commented Sep 28, 2023

NEUpanning commented Sep 28, 2023

NEUpanning commented Sep 29, 2023

DaveCTurner Sep 29, 2023

NEUpanning Oct 1, 2023

DaveCTurner left a comment

DaveCTurner commented Oct 1, 2023

NEUpanning commented Oct 1, 2023 •

edited

NEUpanning commented Oct 1, 2023

DaveCTurner commented Oct 1, 2023

NEUpanning commented Oct 1, 2023

DaveCTurner commented Oct 1, 2023

NEUpanning commented Oct 2, 2023

DaveCTurner left a comment

DaveCTurner left a comment

DaveCTurner commented Oct 2, 2023

NEUpanning commented Oct 2, 2023

DaveCTurner commented Oct 2, 2023

NEUpanning commented Oct 2, 2023

NEUpanning commented Oct 7, 2023

DaveCTurner commented Oct 7, 2023

DaveCTurner commented Oct 16, 2023

NEUpanning commented Oct 16, 2023

Prune unnecessary information from TransportNodesInfoAction.NodeInfoRequest #99938

Prune unnecessary information from TransportNodesInfoAction.NodeInfoRequest #99938

Conversation

NEUpanning commented Sep 27, 2023 • edited

elasticsearchmachine commented Sep 27, 2023

DaveCTurner commented Sep 27, 2023

NEUpanning commented Sep 27, 2023

DaveCTurner commented Sep 27, 2023

NEUpanning commented Sep 27, 2023

NEUpanning commented Sep 28, 2023

NEUpanning commented Sep 28, 2023

DaveCTurner commented Sep 28, 2023

NEUpanning commented Sep 28, 2023

NEUpanning commented Sep 29, 2023

DaveCTurner Sep 29, 2023

Choose a reason for hiding this comment

NEUpanning Oct 1, 2023

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner commented Oct 1, 2023

NEUpanning commented Oct 1, 2023 • edited

NEUpanning commented Oct 1, 2023

DaveCTurner commented Oct 1, 2023

NEUpanning commented Oct 1, 2023

DaveCTurner commented Oct 1, 2023

NEUpanning commented Oct 2, 2023

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner commented Oct 2, 2023

NEUpanning commented Oct 2, 2023

DaveCTurner commented Oct 2, 2023

NEUpanning commented Oct 2, 2023

NEUpanning commented Oct 7, 2023

DaveCTurner commented Oct 7, 2023

DaveCTurner commented Oct 16, 2023

NEUpanning commented Oct 16, 2023

NEUpanning commented Sep 27, 2023 •

edited

NEUpanning commented Oct 1, 2023 •

edited