Pausable chunked HTTP responses #104851
Pausable chunked HTTP responses #104851
Conversation
Today we must collect together in memory everything needed to send a HTTP response before starting to send it to the client, even if we're using the `chunked` transfer encoding to bound the memory needed for the final serialization step. To properly bound the memory usage on the coordinating node we must instead be able to start sending the response to the client before we have collected everything needed to finish it. If we do this then we must be able to handle the case where we run out of data to send by pausing the transmission and resuming it once there's more data to send. This commit extends the `ChunkedRestResponseBody` interface to allow it to express that it has run out of chunks for immediate transmission, but that the body will continue at a later point.
DaveCTurner
added
>non-issue
WIP
:Distributed Coordination/Network
...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/rest/ChunkedRestResponseBody.java
Show resolved
Hide resolved
|
This is ready to review. However, it has no concrete use cases yet so I'm undecided about whether to merge it now or wait until we actually need it for something. Opinions welcome. |
|
I could try to get some forms of ES|QL's response to come back with it before too long. The Arrow support in #104877 probably won't be soon. But we could use it for the "row-based" or "text" responses from ES|QL. |
There was a problem hiding this comment.
I have not read the tests closely yet. The main code change looks fine to me within my range of understanding.
Could you please confirm that the change should not increase the duration that a channel is held for completing a response? Today, a channel associated to the client request cannot do anything else before the response is fully computed. With this change, we amortize the channel waiting time throughout the response computation so that it is able to send out partial response more quickly without increasing the total time needed for complete processing.
...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java
Outdated
Show resolved
Hide resolved
...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java
Show resolved
Hide resolved
Right. Or at least, this PR doesn't change anything, there are no actions in production which use the pausability yet. But if we made an existing action pausable then it could start sending its response sooner than it does today, but it'd still take the same amount of work to finish computing & sending the response. |
| @@ -39,10 +43,36 @@ public interface ChunkedRestResponseBody { | |||
| Logger logger = LogManager.getLogger(ChunkedRestResponseBody.class); | |||
There was a problem hiding this comment.
I wonder if we should change the name of this interface to ChunkedRestResponseBodyPart? That would make isDone less ambiguous (naming-wise). We could also rename isDone to isPartDone to make this more explicit.
I think the javadoc above could also be improved to mention that it can be multi-part.
There was a problem hiding this comment.
++ I will do this in a follow-up to avoid introducing too much noise here. I pushed 4570c92 to improve the Javadocs here tho.
...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/http/DefaultRestChannelTests.java
Outdated
Show resolved
Hide resolved
DaveCTurner
added
the
auto-merge-without-approval
Follow-up to elastic#104851 to rename some symbols to reflect that the class formerly known as a `ChunkedRestResponseBody` may now only be _part_ of the whole response body.
The listener supplied to `getNextPart()` may be completed in a non-default thread context, and this trips assertions about thread context pollution in the transport layer, so this commit adds the missing protection against that. Also the listener supplied to `getNextPart()` may be completed immediately on the transport thread, which could lead to a stack overflow, so this commit adds another `execute()` call to unconditionally fork a fresh task. Relates elastic#104851
Follow-up to elastic#104851 to rename some symbols to reflect that the class formerly known as a `ChunkedRestResponseBody` may now only be _part_ of the whole response body.
The listener supplied to `getNextPart()` may be completed in a non-default thread context, and this trips assertions about thread context pollution in the transport layer, so this commit adds the missing protection against that. Also the listener supplied to `getNextPart()` may be completed immediately on the transport thread, which could lead to a stack overflow, so this commit adds another `execute()` call to unconditionally fork a fresh task. Relates #104851
Adds a utility for implementing REST APIs which construct a streaming (i.e. pretty-much-constant-memory) `.zip` file response as a (pausable) sequence of `ChunkedRestResponseBodyPart` instances, where each entry in the `.zip` file is itself a (pausable) sequence of `ChunkedRestResponseBodyPart` instances. Relates elastic#104851
Adds a utility for implementing REST APIs which construct a streaming (i.e. pretty-much-constant-memory) `.zip` file response as a (pausable) sequence of `ChunkedRestResponseBodyPart` instances, where each entry in the `.zip` file is itself a (pausable) sequence of `ChunkedRestResponseBodyPart` instances. Relates #104851
Adds a utility for implementing REST APIs which construct a streaming (i.e. pretty-much-constant-memory) `.zip` file response as a (pausable) sequence of `ChunkedRestResponseBodyPart` instances, where each entry in the `.zip` file is itself a (pausable) sequence of `ChunkedRestResponseBodyPart` instances. Relates elastic#104851
Adds a utility for implementing REST APIs which construct a streaming (i.e. pretty-much-constant-memory) `.zip` file response as a (pausable) sequence of `ChunkedRestResponseBodyPart` instances, where each entry in the `.zip` file is itself a (pausable) sequence of `ChunkedRestResponseBodyPart` instances. Relates #104851
Adds a utility for implementing REST APIs which construct a streaming (i.e. pretty-much-constant-memory) `.zip` file response as a (pausable) sequence of `ChunkedRestResponseBodyPart` instances, where each entry in the `.zip` file is itself a (pausable) sequence of `ChunkedRestResponseBodyPart` instances. Relates elastic#104851
Today we must collect together in memory everything needed to send a
HTTP response before starting to send it to the client, even if we're
using the
chunkedtransfer encoding to bound the memory needed for thefinal serialization step.
To properly bound the memory usage on the coordinating node we must
instead be able to start sending the response to the client before we
have collected everything needed to finish it. If we do this then we
must be able to handle the case where we run out of data to send by
pausing the transmission and resuming it once there's more data to send.
This commit extends the
ChunkedRestResponseBodyinterface to allow itto express that it has run out of chunks for immediate transmission, but
that the body will continue at a later point.