Flatten Netty4 http pipeline handlers into a single handler #86133
Conversation
This sets up the implementation of chunked REST response encoding that will be able to send large responses without serializing them in full on heap upfront. Having all the steps of sending a response in one place together with the pipelining logic allows adding another response type that serializes on the transport threads alongside `Netty4HttpResponse` in a follow-up that will only require changes to `Netty4PipeliningHandler`. Also, this change in isolation saves loads of indirection and now (that the NIO transport is gone) redundant abstractions, making the path of sending a REST response all around easier to follow.
original-brownbear
added
:Distributed Coordination/Network
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
original-brownbear
requested review from
DaveCTurner,
Tim-Brooks and
henningandersen
...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java
Outdated
Show resolved
Hide resolved
| Thread.sleep(scaledRandomIntBetween(500, 1000)); | ||
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); | ||
| public void incomingRequest(HttpRequest httpRequest, HttpChannel httpChannel) { |
There was a problem hiding this comment.
This change puzzled me a bit in that you tried to keep most other things unchanged, but here went for a more radical change. I see the need for changes (using deleted types), but for my education perhaps you can explain the need for the radical change of the test.
There was a problem hiding this comment.
Sure :) See this change: https://github.com/elastic/elasticsearch/pull/86133/files#diff-5fb205d075055657d087e3445ff349bcfed01425594e604f7bf7b3d3a6dfefa9L325
we had the pipelining handler and then the handler as separate channel handlers before, now they're a single handler. So previously the test could just replace the "handler" with PossiblySlowUpstreamHandler and still test the logic in the unchanged "pipelining" handler. Now that both handlers are one, this wasn't possible any more.
So since I couldn't replace part of the handler, I left the pipeline as is and overrode the http server transport to catch the requests a little further down the stack.
Or put differently, we previously replaced this code https://github.com/elastic/elasticsearch/pull/86133/files#diff-a3368fd4145592d305c99a7c2b0a6c21c9bd6d45c828515aed1f228b69af2a79L29 in the test, now we run it in this test as well and overrode the code it calls.
There was a problem hiding this comment.
Thanks for the explanation!
|
Thanks Henning, will do! |
3 participants
This sets up the implementation of chunked REST response encoding
that will be able to send large responses without serializing them
in full on heap upfront.
Having all the steps of sending a response in one place together with
the pipelining logic allows adding another response type that serializes
on the transport threads alongside
Netty4HttpResponsein a follow-upthat will only require changes to
Netty4PipeliningHandler.Also, this change in isolation saves loads of indirection and now (that the NIO
transport is gone) redundant abstractions, making the path of sending a REST
response all around easier to follow.
relates #82085
NOTE: I did copy a lot of the moved around code outright without too much renaming. While the resulting code could be made nicer by renaming and refactoring the way the methods are set up, I figured for this step it's far easier to review things if I copy as much as I can outright. The same reasoning went into the rather awkward way in which I made
Netty4HttpPipeliningHandlerTestswork for now. A follow-up aligning it better withNetty4HttpServerPipeliningTestsshould be able to clean this up further.