Decouple nio constructs from the tcp transport #27484

Tim-Brooks · 2017-11-21T20:30:48Z

This is related to #27260. Currently, basic nio constructs (nio
channels, the channel factories, selector event handlers, etc) implement
logic that is specific to the tcp transport. For example, NioChannel
implements the TcpChannel interface. These nio constructs at some point
will also need to support other protocols (ex: http).

This commit separates the TcpTransport logic from the nio building
blocks.

Tim-Brooks · 2017-11-21T20:31:04Z

This will also go on the 6.x branch. But there is not yet a tag for 6.2.

s1monw

lefts some minors LGTM mostly

s1monw · 2017-11-21T21:28:37Z

test/framework/src/main/java/org/elasticsearch/transport/nio/AcceptorEventHandler.java

@@ -79,8 +71,8 @@ void acceptChannel(NioServerSocketChannel nioServerChannel) throws IOException {
        ChannelFactory channelFactory = nioServerChannel.getChannelFactory();
        SocketSelector selector = selectorSupplier.get();
        NioSocketChannel nioSocketChannel = channelFactory.acceptNioChannel(nioServerChannel, selector);
-        openChannels.acceptedChannelOpened(nioSocketChannel);
-        acceptedChannelCallback.accept(nioSocketChannel);
+//        openChannels.acceptedChannelOpened(nioSocketChannel);


s1monw · 2017-11-22T12:32:55Z

test/framework/src/main/java/org/elasticsearch/transport/nio/NioTransport.java

+    private void acceptChannel(NioSocketChannel channel) {
+        TcpNioSocketChannel tcpChannel = (TcpNioSocketChannel) channel;
+        openChannels.acceptedChannelOpened(tcpChannel);
+        tcpChannel.addCloseListener(new RemoveClosedChannelListener(channel));


rather use ActionListener.wrap(v -> openChannels.channelClosed(channel), e -> openChannels.channelClosed(channel)) then we don't need to define this extra class

Yes. And better than that, I can used the Runnable version ActionListener.wrap(() -> openChannels.channelClosed(channel))

s1monw · 2017-11-22T12:34:14Z

test/framework/src/main/java/org/elasticsearch/transport/nio/channel/ChannelFactory.java

@@ -34,76 +32,86 @@
 import java.nio.channels.SocketChannel;
 import java.util.function.Consumer;

-public class ChannelFactory {
+public abstract class ChannelFactory<SS extends NioServerSocketChannel, S extends NioSocketChannel> {


can we use different generics s/SS/ServerSocket/ s/S/Socket please?

s1monw · 2017-11-22T12:34:56Z

test/framework/src/main/java/org/elasticsearch/transport/nio/channel/ChannelFactory.java

+     */
+    public abstract SS createServerChannel(AcceptingSelector selector, ServerSocketChannel channel) throws IOException;
+
+    private S createChannel0(SocketSelector selector, SocketChannel rawChannel) throws IOException {


we usually call it internalCreateChannel instead of using a number

s1monw · 2017-11-22T12:45:41Z

test/framework/src/main/java/org/elasticsearch/transport/nio/channel/NioChannel.java

@@ -43,4 +45,6 @@
    SelectionKey getSelectionKey();

    NetworkChannel getRawChannel();
+
+    void addCloseListener(ActionListener<Void> listener);


javadocs please :)

s1monw · 2017-11-22T12:46:18Z

test/framework/src/main/java/org/elasticsearch/transport/nio/channel/NioSocketChannel.java

@@ -39,6 +39,7 @@
    private final SocketSelector socketSelector;
    private WriteContext writeContext;
    private ReadContext readContext;
+    private BiConsumer<NioSocketChannel, Exception> exceptionHandler;


can we ensure that this is only set once?

s1monw · 2017-11-22T12:46:34Z

test/framework/src/main/java/org/elasticsearch/transport/nio/channel/TcpChannelFactory.java

+import java.nio.channels.SocketChannel;
+import java.util.function.Consumer;
+
+public class TcpChannelFactory extends ChannelFactory<TcpNioServerSocketChannel, TcpNioSocketChannel> {


can this be pkg private? and can we have javadocs?

It cannot be package private as it is instantiated in a NioTransport which is in a different package. I think at some point I will rework the packages and it can eventually be package private. But that is a different PR.

s1monw · 2017-11-22T12:46:58Z

test/framework/src/main/java/org/elasticsearch/transport/nio/channel/TcpChannelFactory.java

+        this.serverContextSetter = serverContextSetter;
+    }
+
+    public TcpNioSocketChannel createChannel(SocketSelector selector, SocketChannel channel) throws IOException {


@Override?

s1monw · 2017-11-22T12:47:11Z

...amework/src/main/java/org/elasticsearch/transport/nio/channel/TcpNioServerSocketChannel.java

+import java.io.IOException;
+import java.nio.channels.ServerSocketChannel;
+
+public class TcpNioServerSocketChannel extends NioServerSocketChannel implements TcpChannel {


s1monw · 2017-11-22T12:47:45Z

test/framework/src/main/java/org/elasticsearch/transport/nio/channel/TcpNioSocketChannel.java

+
+    @Override
+    public void setSoLinger(int value) throws IOException {
+        if (isOpen()) {


does this need to be protected to be not concurrently closed?

I'm not sure there is a clear way to do that. If it is closed, it just throws an IOException. And we handle / log that. Additionally, this only occurs at transport shutdown (as setting SO_LINGER is mainly to avoid running out of ports during the build).

This is related to #27260. Currently, basic nio constructs (nio channels, the channel factories, selector event handlers, etc) implement logic that is specific to the tcp transport. For example, NioChannel implements the TcpChannel interface. These nio constructs at some point will also need to support other protocols (ex: http). This commit separates the TcpTransport logic from the nio building blocks.

* es/master: (38 commits) Backport wait_for_initialiazing_shards to cluster health API Carry over version map size to prevent excessive resizing (#27516) Fix scroll query with a sort that is a prefix of the index sort (#27498) Delete shard store files before restoring a snapshot (#27476) Replace `delimited_payload_filter` by `delimited_payload` (#26625) CURRENT should not be a -SNAPSHOT version if build.snapshot is false (#27512) Fix merging of _meta field (#27352) Remove unused method (#27508) unmuted test, this has been fixed by #27397 Consolidate version numbering semantics (#27397) Add wait_for_no_initializing_shards to cluster health API (#27489) [TEST] use routing partition size based on the max routing shards of the second split Adjust CombinedDeletionPolicy for multiple commits (#27456) Update composite-aggregation.asciidoc Deprecate `levenstein` in favor of `levenshtein` (#27409) Automatically prepare indices for splitting (#27451) Validate `op_type` for `_create` (#27483) Minor ShapeBuilder cleanup muted test Decouple nio constructs from the tcp transport (#27484) ...

* es/6.x: (30 commits) Add wait_for_no_initializing_shards to cluster health API (#27489) Carry over version map size to prevent excessive resizing (#27516) Fix scroll query with a sort that is a prefix of the index sort (#27498) Delete shard store files before restoring a snapshot (#27476) CURRENT should not be a -SNAPSHOT version if build.snapshot is false (#27512) Fix merging of _meta field (#27352) test: do not run percolator query builder bwc test against 5.x versions Remove unused method (#27508) Consolidate version numbering semantics (#27397) Adjust CombinedDeletionPolicy for multiple commits (#27456) Minor ShapeBuilder cleanup [GEO] Deprecate ShapeBuilders and decouple geojson parse logic Improve docs for split API in 6.1/6.x (#27504) test: use correct pre 6.0.0-alpha1 format Update composite-aggregation.asciidoc Deprecate `levenstein` in favor of `levenshtein` (#27409) Decouple nio constructs from the tcp transport (#27484) Bump version from 6.1 to 6.2 Fix whitespace in Security.java Tighten which classes can exit ...

Tim-Brooks added 6 commits November 20, 2017 17:17

WIP

b35a1a8

WIP

23844d3

WIP

187f870

Remove imports

81ec8a9

WIP

bb38080

WIP

4287cad

Tim-Brooks added :Distributed Coordination/Network Http and internode communication implementations >enhancement review v7.0.0 labels Nov 21, 2017

Tim-Brooks requested review from s1monw, jaymode and jasontedor November 21, 2017 20:30

Tim-Brooks changed the title ~~Decouple Nio constructs from the tcp transport~~ Decouple nio constructs from the tcp transport Nov 21, 2017

Remove comment

a8f1fe1

s1monw approved these changes Nov 22, 2017

View reviewed changes

Tim-Brooks added 2 commits November 22, 2017 09:40

Changes based on review

e0420a7

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

6cfbb2e

Tim-Brooks added the v6.2.0 label Nov 22, 2017

Tim-Brooks merged commit ef34555 into elastic:master Nov 22, 2017

Tim-Brooks deleted the fix_types2 branch November 14, 2018 14:51

jimczi 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

Decouple nio constructs from the tcp transport #27484

Decouple nio constructs from the tcp transport #27484

Tim-Brooks commented Nov 21, 2017

Tim-Brooks commented Nov 21, 2017

s1monw left a comment

s1monw Nov 21, 2017

s1monw Nov 22, 2017

Tim-Brooks Nov 22, 2017

s1monw Nov 22, 2017

s1monw Nov 22, 2017

s1monw Nov 22, 2017

s1monw Nov 22, 2017

s1monw Nov 22, 2017

Tim-Brooks Nov 22, 2017

s1monw Nov 22, 2017

s1monw Nov 22, 2017

s1monw Nov 22, 2017

Tim-Brooks Nov 22, 2017

Decouple nio constructs from the tcp transport #27484

Decouple nio constructs from the tcp transport #27484

Conversation

Tim-Brooks commented Nov 21, 2017

Tim-Brooks commented Nov 21, 2017

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