Convert BytesTransportResponse when proxying response from/to local node by javanna · Pull Request #135873 · elastic/elasticsearch

javanna · 2025-10-02T19:23:29Z

#127112 introduced BytesTransportResponse to be used in search batched execution, so that NodeQueryResponse could be written as BytesTransportResponse as opposed to materializing the response object on heap.

When a proxy node acts as a proxy to query its local data, and the coordinating node is on a different version than the proxy node, the response will fail to deserialize in the coord node because it was written with the version of the proxy node as opposed to that of the coord (target) node. This is because DirectResponseChannel skips the step of reading and writing back such response, which would lead to it being converted to the right format.

This commit attempts to fix this problem by tracking the version used to write the response, and conditionally converting it in the ProxyRequestHandler.

There may be better ways to fix this problem, let's use this draft to align on the problem and agree on possible solutions. I am happy to iterate on it, as this currently blocks merging #135549 due to the serialization change it introduces.

Note that search batched executed is still under a feature flag, and disabled by default, so this bug does not currently affect our users.

execution, to a BytesTransportResponse as opposed to materializing the response object on heap. When a proxy node acts as a proxy to query its local data, and the coordinating node is on a different version than the proxy node, the response will fail to deserialize in the coord node because it was written with the version of the proxy node as opposed to that of the coord (target) node. This is because DirectResponseChannel does not read and write such response, which would lead to it being converted to the right format. This commit attempts to fix this problem by tracking the version used to write the response, and conditionally converting it in the ProxyRequestHandler.

elasticsearchmachine · 2025-10-02T19:23:55Z

Hi @javanna, I've created a changelog YAML for you.

elasticsearchmachine · 2025-10-03T10:09:54Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

DaveCTurner

This deserves some tests in TransportActionProxyTests showing that we do/don't re-serialize the response in these cases.

server/src/main/java/org/elasticsearch/transport/TransportActionProxy.java

…onProxy.java Co-authored-by: David Turner <david.turner@elastic.co>

javanna · 2025-10-03T12:30:45Z

server/src/main/java/org/elasticsearch/transport/TransportActionProxy.java

+                            try {
+                                channel.sendResponse(convertedResponse);
+                            } finally {
+                                convertedResponse.decRef();


I'd initially forgotten this, can you double check that this is what I need to be doing please?

javanna · 2025-10-03T12:31:21Z

server/src/main/java/org/elasticsearch/transport/TransportActionProxy.java

+                                convertedResponse.decRef();
+                            }
+                        } catch (IOException e) {
+                            throw new UncheckedIOException(e);


should I throw or send the exception as a response? I am assuming that exceptions thrown are already caught and sent as responses, but good to double check.

javanna · 2025-10-03T12:45:54Z

server/src/test/java/org/elasticsearch/transport/TransportActionProxyTests.java

        public void writeTo(StreamOutput out) throws IOException {
            out.writeString(targetNode);
+            if (out.getTransportVersion().supports(transportVersion1)) {
+                out.writeBoolean(true);


This reproduces the problem in the tests I added, which is fixed with the conversion this change introduces.

javanna · 2025-10-03T12:47:22Z

server/src/test/java/org/elasticsearch/transport/TransportActionProxyTests.java

+            "internal:test",
+            cancellable,
+            // For a proxy node proxying to itself, the response is sent directly, without it being read by the proxy layer
+            r -> { throw new AssertionError(); },


this diff is misleading! I only changed this line in testSendLocalRequest

javanna · 2025-10-03T12:48:13Z

Thanks for the suggestion, I added tests @DaveCTurner.

DaveCTurner

LGTM

…ode (elastic#135873) elastic#127112 introduced `BytesTransportResponse` to be used in search batched execution, so that `NodeQueryResponse` could be written as `BytesTransportResponse` as opposed to materializing the response object on heap. When a proxy node acts as a proxy to query its local data, and the coordinating node is on a different version than the proxy node, the response will fail to deserialize in the coord node because it was written with the version of the proxy node as opposed to that of the coord (target) node. This is because `DirectResponseChannel` skips the step of reading and writing back such response, which would lead to it being converted to the right format. This commit attempts to fix this problem by tracking the version used to write the binary response, and conditionally converting it in the `ProxyRequestHandler` when the version don't align.

…ocal node (#135873) (#138070) #127112 introduced `BytesTransportResponse` to be used in search batched execution, so that `NodeQueryResponse` could be written as `BytesTransportResponse` as opposed to materializing the response object on heap. When a proxy node acts as a proxy to query its local data, and the coordinating node is on a different version than the proxy node, the response will fail to deserialize in the coord node because it was written with the version of the proxy node as opposed to that of the coord (target) node. This is because `DirectResponseChannel` skips the step of reading and writing back such response, which would lead to it being converted to the right format. This commit attempts to fix this problem by tracking the version used to write the binary response, and conditionally converting it in the `ProxyRequestHandler` when the version don't align. --------- Co-authored-by: Luca Cavanna <javanna@apache.org>

…ocal node (#135873) (#138071) #127112 introduced `BytesTransportResponse` to be used in search batched execution, so that `NodeQueryResponse` could be written as `BytesTransportResponse` as opposed to materializing the response object on heap. When a proxy node acts as a proxy to query its local data, and the coordinating node is on a different version than the proxy node, the response will fail to deserialize in the coord node because it was written with the version of the proxy node as opposed to that of the coord (target) node. This is because `DirectResponseChannel` skips the step of reading and writing back such response, which would lead to it being converted to the right format. This commit attempts to fix this problem by tracking the version used to write the binary response, and conditionally converting it in the `ProxyRequestHandler` when the version don't align. --------- Co-authored-by: Luca Cavanna <javanna@apache.org>

benchaplin · 2025-11-17T19:00:33Z

I've backported this to 9.1/9.2 to support backports of additional transport versions for BytesTransportResponse.

javanna added >bug :Distributed/Network Http and internode communication implementations :Search Foundations/Search Catch all for Search Foundations v9.3.0 labels Oct 2, 2025

Update docs/changelog/135873.yaml

a8b04f2

javanna removed the :Search Foundations/Search Catch all for Search Foundations label Oct 2, 2025

javanna added 4 commits October 2, 2025 23:01

iter

4046fa3

iter

02afc14

Merge branch 'main' into fix/proxy_direct_response_convert

16c46cd

iter

436d394

javanna marked this pull request as ready for review October 3, 2025 10:09

elasticsearchmachine added the Team:Distributed Coordination (obsolete) Meta label for Distributed Coordination team. Obsolete. Please do not use. label Oct 3, 2025

javanna added 2 commits October 3, 2025 12:18

iter

946a6fa

iter

01405da

DaveCTurner reviewed Oct 3, 2025

View reviewed changes

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

javanna and others added 2 commits October 3, 2025 13:09

Update server/src/main/java/org/elasticsearch/transport/TransportActi…

cbc8e21

…onProxy.java Co-authored-by: David Turner <david.turner@elastic.co>

iter

52c7825

javanna commented Oct 3, 2025

View reviewed changes

javanna requested a review from DaveCTurner October 3, 2025 13:06

lkts mentioned this pull request Oct 3, 2025

Add reshardSplitShardCountSummary field to ShardSearchRequest and friends #135804

Merged

javanna added 2 commits October 3, 2025 21:08

Merge branch 'main' into fix/proxy_direct_response_convert

b903d66

Merge branch 'main' into fix/proxy_direct_response_convert

ebee27d

DaveCTurner approved these changes Oct 6, 2025

View reviewed changes

javanna merged commit cebc1a8 into elastic:main Oct 6, 2025
34 checks passed

javanna deleted the fix/proxy_direct_response_convert branch October 6, 2025 12:01

javanna added v8.19.6 and removed v8.19.6 labels Oct 6, 2025

benchaplin mentioned this pull request Nov 14, 2025

[9.2] Convert BytesTransportResponse when proxying response from/to local node (#135873) #138070

Merged

benchaplin mentioned this pull request Nov 14, 2025

[9.1] Convert BytesTransportResponse when proxying response from/to local node (#135873) #138071

Merged

benchaplin added branch:9.2 branch:9.1 labels Nov 17, 2025

elasticsearchmachine added v9.2.2 v9.1.8 and removed branch:9.2 branch:9.1 labels Nov 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert BytesTransportResponse when proxying response from/to local node#135873

Convert BytesTransportResponse when proxying response from/to local node#135873
javanna merged 12 commits intoelastic:mainfrom
javanna:fix/proxy_direct_response_convert

javanna commented Oct 2, 2025 •

edited

Loading

elasticsearchmachine commented Oct 2, 2025

elasticsearchmachine commented Oct 3, 2025

DaveCTurner left a comment

Uh oh!

javanna Oct 3, 2025

javanna Oct 3, 2025

javanna Oct 3, 2025

javanna Oct 3, 2025

javanna commented Oct 3, 2025

DaveCTurner left a comment

Uh oh!

benchaplin commented Nov 17, 2025

Labels

4 participants

Conversation

javanna commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Oct 2, 2025

elasticsearchmachine commented Oct 3, 2025

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

javanna Oct 3, 2025

Choose a reason for hiding this comment

javanna Oct 3, 2025

Choose a reason for hiding this comment

javanna Oct 3, 2025

Choose a reason for hiding this comment

javanna Oct 3, 2025

Choose a reason for hiding this comment

javanna commented Oct 3, 2025

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

benchaplin commented Nov 17, 2025

Labels

4 participants

javanna commented Oct 2, 2025 •

edited

Loading