Extract common http logic to server #31311

Tim-Brooks · 2018-06-13T23:26:28Z

This is related to #28898. With the addition of the http nio transport,
we now have two different modules that provide http transports.
Currently most of the http logic lives at the module level. However,
some of this logic can live in server. In particular, some of the
setting of headers, cors, and pipelining. This commit begins this moving
in that direction by introducing lower level abstraction (HttpChannel,
HttpRequest, and HttpResonse) that is implemented by the modules. The
higher level rest request and rest channel work can live entirely in
server.

elasticmachine · 2018-06-13T23:26:29Z

Pinging @elastic/es-core-infra

Tim-Brooks · 2018-06-13T23:32:29Z

The idea here is that there are now single implementations of RestRequest and RestChannel (technically there are multiple RestChannel implementations - but a single instance DefaultRestChannel is the most important one - opposed to separate implementations per module). Instead of implementing those classes, the lower level http modules provide a HttpChannel (similar to how the transport module provide a TcpChannel). This HttpChannel is connection based, opposed to a different one per request.

Additionally module provide lightweight HttpRequest implementations that allow accessing headers, uri, content, etc and a lightweight HttpResponse that allows setting headers. All of the logic for decoding params, setting headers, etc now lives in AbstractHttpServerTransport and DefaultRestChannel.

Eventually the cors logic and pipelining logic should be moved to server.

s1monw · 2018-06-14T07:59:52Z

meh 2k SLoC - I will try...

s1monw

I do like that we are sharing lots of stuff now. I left some comments on the way mainly style etc. LGTM in general, thanks for doing this

s1monw · 2018-06-14T08:57:11Z

modules/transport-netty4/src/main/java/org/elasticsearch/transport/netty4/Netty4Transport.java

            ch.pipeline().addLast("logging", new ESLoggingHandler());
            ch.pipeline().addLast("size", new Netty4SizeHeaderFrameDecoder());
            ch.pipeline().addLast("dispatcher", new Netty4MessageChannelHandler(Netty4Transport.this, name));
+            serverAcceptedChannel(nettyTcpChannel);


s1monw · 2018-06-14T08:57:59Z

modules/transport-netty4/src/main/java/org/elasticsearch/transport/netty4/NettyTcpChannel.java

@@ -98,8 +98,11 @@ public void sendMessage(BytesReference reference, ActionListener<Void> listener)
            } else {
                final Throwable cause = f.cause();
                Netty4Utils.maybeDie(cause);
-                assert cause instanceof Exception;
-                listener.onFailure((Exception) cause);
+                if (cause instanceof Error) {


wait, what error can get past Netty4Utils.maybeDie(cause);?

None. Elasticsearch should still die on another thread. We just have to do something on this thread in the meantime. In the old version the assertion would only trigger is assertions were enabled. And then it would throw a class cast exception.

I mean this is super edge case, but I thought that elasticsearch dying on another thread would handle the Error and on this thread we would just continue on in the meantime.

s1monw · 2018-06-14T09:00:41Z

plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/NioHttpResponse.java

+    }
+
+    static {
+        EnumMap<RestStatus, HttpResponseStatus> map = new EnumMap<>(RestStatus.class);


can we somehow share this ? I saw it in multiple places now. maybe in HttpResponse?

Yes using the status code (int). I'll do that in a followup though.

s1monw · 2018-06-14T09:01:52Z

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

+ */
+package org.elasticsearch.http;
+
+public abstract class AbstractHttpPipelinedMessage implements HttpPipelinedMessage {


this only has a single implementation. can we fold it into it?

s1monw · 2018-06-14T09:02:32Z

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

@@ -81,6 +90,10 @@ protected AbstractHttpServerTransport(Settings settings, NetworkService networkS
        this.maxContentLength = SETTING_HTTP_MAX_CONTENT_LENGTH.get(settings);
    }

+    public BigArrays getBigArrays() {


can this be protected?

s1monw · 2018-06-14T09:06:29Z

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

-    public int getSequence() {
-        return sequence;
-    }
+    int getSequence();


I am not sure what this is supposed to return. can you add javadocs?

from my guessing I wonder if we should name it getOrd()

s1monw · 2018-06-14T09:07:13Z

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

+
+package org.elasticsearch.http;
+
+public interface HttpResponse {


javadocs pls

s1monw · 2018-06-14T09:07:22Z

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

+import java.util.List;
+import java.util.Map;
+
+public interface HttpRequest {


javadocs pls

s1monw · 2018-06-14T09:08:51Z

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

-    @Nullable
-    public SocketAddress getRemoteAddress() {
-        return null;
+    public InetSocketAddress getRemoteAddress() {


would it make sense to let the consumer just call getHttpChannel and then get from it what they need instead of having these delegates?

s1monw · 2018-06-14T09:10:02Z

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

     */
-    @Nullable
    public Map<String, List<String>> getHeaders() {


* master: 992c788 Uncouple persistent task state and status (#31031) 8c6ee7d Describe how to add a plugin in Dockerfile (#31340) 1c5cec0 Remove http status code maps (#31350) 87a676e Do not set vm.max_map_count when unnecessary (#31285) e5b7137 TEST: getCapturedRequestsAndClear should be atomic (#31312) 0324103 Painless: Fix bug for static method calls on interfaces (#31348) d6d0727 QA: Fix resolution of default distribution (#31351) fcf1e41 Extract common http logic to server (#31311) 6dd81ea Build: Fix the license in the pom zip and tar (#31336) 8f886cd Treat ack timeout more like a publish timeout (#31303) 9b29327 [ML] Add description to ML filters (#31330) f7a0caf SQL: Fix build on Java 10 375d09c [TEST] Fix RemoteClusterClientTests#testEnsureWeReconnect 4877cec More detailed tracing when writing metadata (#31319) bbfe1ec [Tests] Mutualize fixtures code in BaseHttpFixture (#31210)

Tim-Brooks added 20 commits June 7, 2018 13:08

WIP

2fe6a19

WIP

cf8eb63

WIP

ca521f7

WIP

39c7caf

WIP

26b5d7a

Work on transition nio to new server work

6322fa2

Make some adjustments to how responses are created

e713fce

Merge remote-tracking branch 'upstream/master' into default_rest_channel

6333342

Fix checkstyle

bc9154a

Continue getting nio to work with new path

fc04691

Fix tests

42a013e

WIP

2a31aed

Merge remote-tracking branch 'upstream/master' into default_rest_channel

1ed37e6

Merge remote-tracking branch 'upstream/master' into default_rest_channel

50ca17f

Work on netty module integration

9b796d6

WIP

26c1d5e

Alot of renaming

c3639c4

WIP

48fafd2

WIP

e7e63e1

Fix tests

1aac362

Tim-Brooks added >non-issue review :Distributed/Network Http and internode communication implementations v7.0.0 labels Jun 13, 2018

Tim-Brooks requested review from s1monw, jaymode and jasontedor June 13, 2018 23:26

Renname channel

893501d

Tim-Brooks added 4 commits June 13, 2018 17:40

Cleanups

dc03bb2

Fix license

ea79116

Fix head requests

29d2483

Merge remote-tracking branch 'upstream/master' into default_rest_channel

1d7455d

s1monw approved these changes Jun 14, 2018

View reviewed changes

Tim-Brooks added 6 commits June 14, 2018 09:45

Merge remote-tracking branch 'upstream/master' into default_rest_channel

0da1945

Changes from review

d67d6d7

Fix checkstyle

a0948f2

Fix checkstyle

9bc92cd

Fix forbidden

b67825c

Fix test

3a8dfe9

Tim-Brooks merged commit fcf1e41 into elastic:master Jun 14, 2018

Tim-Brooks deleted the default_rest_channel branch December 10, 2018 16:20

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract common http logic to server #31311

Extract common http logic to server #31311

Tim-Brooks commented Jun 13, 2018

elasticmachine commented Jun 13, 2018

Tim-Brooks commented Jun 13, 2018

s1monw commented Jun 14, 2018

s1monw left a comment

s1monw Jun 14, 2018

s1monw Jun 14, 2018

Tim-Brooks Jun 14, 2018

s1monw Jun 14, 2018

Tim-Brooks Jun 14, 2018

s1monw Jun 14, 2018

s1monw Jun 14, 2018

s1monw Jun 14, 2018

s1monw Jun 14, 2018

s1monw Jun 14, 2018

s1monw Jun 14, 2018

s1monw Jun 14, 2018

s1monw Jun 14, 2018


		package org.elasticsearch.http;

		public interface HttpResponse {

Extract common http logic to server #31311

Extract common http logic to server #31311

Conversation

Tim-Brooks commented Jun 13, 2018

elasticmachine commented Jun 13, 2018

Tim-Brooks commented Jun 13, 2018

s1monw commented Jun 14, 2018

s1monw 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

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

Choose a reason for hiding this comment