Skip to content

Conversation

mhl-b
Copy link
Contributor

@mhl-b mhl-b commented Aug 24, 2024

Extend Netty4HttpAggregator to handle 100-continue, 413/417 for partial content. I reuse public API from MessageAggregator that handles 100-continue. For known content length I dont close connection after 413/417. For chunked content there is no limit, rest handler will decide when to stop.

Added integ tests.

@mhl-b mhl-b added >enhancement :Distributed Coordination/Network Http and internode communication implementations Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.16.0 labels Aug 24, 2024
@elasticsearchmachine
Copy link
Collaborator

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

Comment on lines 278 to 279
// ensures that server reply 413-too-large on oversized chunked encoding request and closes connection
public void test413TooLargeOnChunkedEncoding() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right behaviour. We might have already started processing the chunked request, but 413 implies we are completely rejecting it according to RFC 9110 §15.5.14:

The 413 (Content Too Large) status code indicates that the server is refusing to process a request because the request content is larger than the server is willing or able to process.

Instead, we need to let the RestHandler deal with this situation, so that different handlers can respond in different ways. For instance the _bulk API should report the usual doc-level responses for any successfully-processed docs, and should return a doc-level 429 for docs past the limit. Other APIs that aggregate the whole request up-front can still reasonably return a 413.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I remember, we talked about it. I tried to make it similar to what http-object-aggregator does. I will remove rejection for chunked request.

@mhl-b mhl-b requested a review from DaveCTurner August 26, 2024 19:01
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhl-b mhl-b merged commit db5c1b0 into elastic:partial-rest-requests Aug 26, 2024
Tim-Brooks pushed a commit to Tim-Brooks/elasticsearch that referenced this pull request Sep 19, 2024
Tim-Brooks added a commit that referenced this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed Coordination/Network Http and internode communication implementations >enhancement Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants