Use chunked REST serialization for large REST responses #88311

original-brownbear · 2022-07-06T13:16:30Z

Adds chunked rest serialization infrastructure that tries to serialize
only what can be flushed to the channel right away instead of fully
materializing a response on heap first and then writing it to the channel.

Makes use of the new infrastructure for get-snapshots as an example use case.
Follow-ups will extend the usage of this logic to other problematic APIs like
/_cluster/state.

A working version of this with the cluster state crudely chunked can be found in https://github.com/original-brownbear/elasticsearch/pull/new/chunked-rest-response-with-cs

relates #77466

Adds chunked rest serialization infrastructure that tries to serialize only what can be flushed to the channel right away instead of fully materializing a response on heap first and then writing it to the channel. Makes use of the new infrastructure for get-snapshots as an example use case. Follow-ups will extend the usage of this logic to other problematic APIs like `/_cluster/state`.

henningandersen

I went through one more round here and left a few more comments now that I looked at tests too.

...ort-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandlerTests.java

henningandersen · 2022-08-29T09:53:18Z

...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java

+        );
+        final ByteBuf content = Netty4Utils.toByteBuf(bytes);
+        final boolean done = body.isDone();
+        final ChannelFuture f = ctx.write(done ? new DefaultLastHttpContent(content) : new DefaultHttpContent(content));


Can we assert that:

assert done || length > 0;

to ensure progress of chunking in the wide variety of implementations we will eventually see?

We can be more extreme IMO and just assert length > 0 and done == false before that. We shouldn't be calling this once we're done and should never emit an empty buffer not even for the last invocation because that'll just be needless overhead for an empty last chunk. Adding those assertions

I am not sure we can always have that guarantee for the last chunk, but as long as we emit json it sounds plausible so ok by me for now.

henningandersen · 2022-08-29T09:54:31Z

...ort-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandlerTests.java

+        assertTrue(promise.isDone());
+        assertThat(messagesSeen, hasSize(chunks + 1));
+        assertThat(messagesSeen.get(0), instanceOf(Netty4ChunkedHttpResponse.class));
+        assertThat(messagesSeen.get(chunks), instanceOf(LastHttpContent.class));


I'd find it nice to also validate that the bytes we see are right.

Pushed dcf73e4 for this

henningandersen · 2022-08-29T09:57:25Z

...ort-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandlerTests.java

+        assertThat(messagesSeen.get(0), instanceOf(Netty4ChunkedHttpResponse.class));
+        assertThat(messagesSeen.get(chunks), instanceOf(LastHttpContent.class));


Can we verify the in between messages too? At least in this first basic test, but perhaps we can add a helper method to include that in all tests?

Yes pushed dcf73e4 for this that adds that helper method

...ort-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandlerTests.java

henningandersen · 2022-08-29T10:06:22Z

...ort-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandlerTests.java

+        final HttpResponse response1 = request1.createResponse(RestStatus.OK, getRepeatedChunkResponseBody(chunks1, chunk));
+        final HttpResponse response2 = request2.createResponse(RestStatus.OK, getRepeatedChunkResponseBody(chunks2, chunk));
+        final ChannelPromise promise1 = embeddedChannel.newPromise();
+        embeddedChannel.write(response1, promise1);


Can we make this (and the other tests) randomly write the second response first?

Sure added some randomisation to that effect in 911e91d

...ort-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandlerTests.java

henningandersen · 2022-08-29T10:26:32Z

server/src/main/java/org/elasticsearch/rest/ChunkedRestResponseBody.java

+     * @param channel channel the response will be written to
+     * @return chunked rest response body
+     */
+    static ChunkedRestResponseBody fromXContent(ChunkedToXContent chunkedToXContent, ToXContent.Params params, RestChannel channel)


Can we add a test of this method as well as the encodeChunk method?

Sure thing, how about 2943f1b ?

henningandersen · 2022-08-29T10:29:42Z

...oke-test-http/src/javaRestTest/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java

@@ -322,6 +322,33 @@ public void testSortAfterStartTime() throws Exception {
        assertThat(allBeforeStartTimeDescending(startTime1 - 1), empty());
    }

+    public void testLargeChunkedResponses() throws Exception {


I think there is still value in this. For one, encodeChunk does not run there. Not saying that that integration will lead to issues (I see none), but some verification of pipelining with chunking end-to-end seems valuable.

original-brownbear · 2022-09-01T12:00:37Z

Thanks again Henning! I think I once again addressed all points :)

henningandersen

LGTM.

Left a few comments that I hope you will address too.

...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java

henningandersen · 2022-09-06T07:38:46Z

...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java

+    }
+
+    private void finishChunkedWrite() {
+        currentChunkedWrite.combiner.finish(currentChunkedWrite.onDone);


I think netty does catch all here, but would feel safer to clear currentChunkedWrite and increment writeSequence either before calling finish or in a finally in case any of the listeners throws.

Yea unless we have a bug where we finish the same thing twice we shouldn't get any throwing here, but makes sense to me still -> put a finally here.

...ort-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandlerTests.java

henningandersen · 2022-09-06T07:44:41Z

...oke-test-http/src/javaRestTest/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java

@@ -322,6 +322,33 @@ public void testSortAfterStartTime() throws Exception {
        assertThat(allBeforeStartTimeDescending(startTime1 - 1), empty());
    }

+    public void testLargeChunkedResponses() throws Exception {


OK to skip for now. I only thought this might be possible to provoke by doing random requests in parallel (i.e., serially but without waiting for the response) on the same client, but it might not support pipelining.

server/src/main/java/org/elasticsearch/http/DefaultRestChannel.java

henningandersen · 2022-09-06T07:58:27Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java

+                    channel.sendResponse(
+                        new RestResponse(
+                            RestStatus.OK,
+                            (Objects.requireNonNullElse(request.getXContentType(), XContentType.JSON)).mediaType(),


ChunkedRestResponseBody finds the xcontent-type through AbstractRestChannel.newBuilder, can we instead use that by building the ChunkedRestResponseBody first?

Edit: nevermind will fix this, found a neat way to do so

DaveCTurner

Sorry I haven't had a chance to look at this in more detail. I have one question tho: what happens if we add a response header (especially a deprecation warning) during REST serialization, which might now be after we've finished sending response headers to the client? I don't think we can do much in practice but I think it'd be wise to at least cause a test failure if this happens.

original-brownbear · 2022-09-06T10:45:16Z

@DaveCTurner

if we add a response header (especially a deprecation warning) during REST serialization, which might now be after we've finished sending response headers to the client

Good point. We could add these as trailers though, which should be understood by most clients? Maybe in a follow-up that I could get on asap?

original-brownbear · 2022-09-06T10:45:30Z

Jenkins run elasticsearch-ci/part-1 ( known + unrelated)

DaveCTurner · 2022-09-06T11:43:44Z

We could add these as trailers though

TIL yet another dark corner of the HTTP spec...

original-brownbear · 2022-09-06T12:31:18Z

Alrighty, I'm going to merge this one. Thanks Henning + David! :) I'll follow up for the footer thing before I use this for anything that adds headers (GetSnapshotsResponse seems safe to me).

sethmlarson · 2022-09-19T13:43:06Z

@original-brownbear @DaveCTurner I would avoid using trailers if possible, many HTTP libraries don't process them or make them available to users so wouldn't be actionable from clients team.

DaveCTurner · 2022-09-20T06:47:00Z

avoid using trailers if possible

Let's continue this on #89839.

original-brownbear added 30 commits June 20, 2022 15:11

getting there

63dc225

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

d542b74

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

b323fd9

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

ba7cdea

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

fa63c8c

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

f9a3215

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

1ad294d

checkpoint

f022190

cp

a87cfb3

cp

75b036d

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

299cdba

fix compilation

333da0d

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

d9b1f8a

works better

84dd503

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

6077797

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

a95590e

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

07bfaab

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

21cdd21

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

00ee6a0

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

6c3b963

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

9f86863

bck

5b9219a

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

582dfe8

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

5f50d87

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

8a28b04

cleanup test

d327f2d

simpler API

649481e

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

144c227

Merge remote-tracking branch 'elastic/master' into chunked-rest-response

c061b73

original-brownbear requested a review from henningandersen August 28, 2022 11:08

henningandersen reviewed Aug 29, 2022

View reviewed changes

original-brownbear added 8 commits September 1, 2022 11:36

Merge remote-tracking branch 'elastic/main' into chunked-rest-response

f874c72

add asssertions

cc312d7

CR: test adjustments

1154131

CR: test adjustments

6516482

validate response body fully

dcf73e4

randomize write order

911e91d

test the other way around

98ff648

add ChunkedRestResponseBodyTests

2943f1b

original-brownbear requested a review from henningandersen September 1, 2022 12:00

henningandersen approved these changes Sep 6, 2022

View reviewed changes

original-brownbear added 3 commits September 6, 2022 10:03

Merge remote-tracking branch 'elastic/main' into chunked-rest-response

ca30f8c

CR: comments

9743942

single point for content type

99a5aa1

DaveCTurner reviewed Sep 6, 2022

View reviewed changes

original-brownbear merged commit 06d4ef7 into elastic:main Sep 6, 2022

original-brownbear deleted the chunked-rest-response branch September 6, 2022 12:32

This was referenced Sep 6, 2022

Make use of chunked REST response infrastructure in more APIs #89838

Open

Add HTTP trailers to chunked REST infrastructure #89839

Open

DaveCTurner mentioned this pull request Nov 30, 2022

Chunked HTTP APIs send (final/empty) chunk in response to HEAD requests #92032

Closed

original-brownbear restored the chunked-rest-response branch April 18, 2023 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use chunked REST serialization for large REST responses #88311

Use chunked REST serialization for large REST responses #88311

original-brownbear commented Jul 6, 2022 •

edited

Loading

henningandersen left a comment

henningandersen Aug 29, 2022

original-brownbear Sep 1, 2022

henningandersen Sep 6, 2022 •

edited

Loading

henningandersen Aug 29, 2022

original-brownbear Sep 1, 2022

henningandersen Aug 29, 2022

original-brownbear Sep 1, 2022

henningandersen Aug 29, 2022

original-brownbear Sep 1, 2022

henningandersen Aug 29, 2022

original-brownbear Sep 1, 2022 •

edited

Loading

henningandersen Aug 29, 2022

original-brownbear commented Sep 1, 2022

henningandersen left a comment

henningandersen Sep 6, 2022

original-brownbear Sep 6, 2022

henningandersen Sep 6, 2022

henningandersen Sep 6, 2022

original-brownbear Sep 6, 2022 •

edited

Loading

DaveCTurner left a comment

original-brownbear commented Sep 6, 2022

original-brownbear commented Sep 6, 2022

DaveCTurner commented Sep 6, 2022

original-brownbear commented Sep 6, 2022

sethmlarson commented Sep 19, 2022

DaveCTurner commented Sep 20, 2022

		assertThat(messagesSeen.get(0), instanceOf(Netty4ChunkedHttpResponse.class));
		assertThat(messagesSeen.get(chunks), instanceOf(LastHttpContent.class));

Use chunked REST serialization for large REST responses #88311

Use chunked REST serialization for large REST responses #88311

Conversation

original-brownbear commented Jul 6, 2022 • edited Loading

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningandersen Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Sep 1, 2022

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

original-brownbear commented Sep 6, 2022

original-brownbear commented Sep 6, 2022

DaveCTurner commented Sep 6, 2022

original-brownbear commented Sep 6, 2022

sethmlarson commented Sep 19, 2022

DaveCTurner commented Sep 20, 2022

original-brownbear commented Jul 6, 2022 •

edited

Loading

henningandersen Sep 6, 2022 •

edited

Loading

original-brownbear Sep 1, 2022 •

edited

Loading

original-brownbear Sep 6, 2022 •

edited

Loading