Remove connectToNodeLight and replace it with a connection profile #21799

s1monw · 2016-11-25T15:18:04Z

The Transport#connectToNodeLight concepts is confusing and not very flexible.
neither really testable on a unittest level. This commit cleans up the code used
to connect to nodes and simplifies transport implementations to share more code.
This also allows to connect to nodes with custom profiles if needed, for instance
future improvements can be added to connect to/from nodes that are non-data nodes without
dedicated bulks and recovery connections.

The Transport#connectToNodeLight concepts is confusing and not very flexible. neither really testable on a unittest level. This commit cleans up the code used to connect to nodes and simplifies transport implemenations to share more code. This also allows to connect to nodes with custom profiles if needed, for instance future improvements can be added to connect to/from nodes that are non-data nodes without dedicated bulks and recovery connections.

s1monw · 2016-11-25T16:15:26Z

test this please

tlrx · 2016-11-25T16:43:32Z

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

+                for (int i = 0; i < channels.length; i++) {
+                    assert iterator.hasNext();
+                    ChannelFuture future = iterator.next();
+                    future.awaitUninterruptibly((long) (connectTimeout.millis() * 1.5));


Maybe it could be nice to have a connection timeout configurable by ConnectionProfile? Like not using 30s timeouts when we ping nodes every 3s (see #19719 (comment))

good idea, very good idea. I will create a followup for this WDYT @tlrx

@tlrx I looked into this and one problem is that the connect timeout is per Bootstrap on the netty layer. This essentially means we need a dedicated Bootstrap for each connection timeout value. I don't think we should do that, maybe there is a better solution to this? I wonder if we should look into adding this option to netty?

@tlrx scratch that, I think I found a way to make this work, it's a bit involved so it will definitely a sep. PR

@tlrx is it that what you had in mind

@s1monw yes

s1monw · 2016-11-27T07:52:10Z

test this please

s1monw · 2016-11-28T14:11:12Z

as a followup I'd like to do stuff like this:

diff --git a/core/src/main/java/org/elasticsearch/transport/TcpTransport.java b/core/src/main/java/org/elasticsearch/transport/TcpTransport.java
index 66dd217..e0c1d3a 100644
--- a/core/src/main/java/org/elasticsearch/transport/TcpTransport.java
+++ b/core/src/main/java/org/elasticsearch/transport/TcpTransport.java
@@ -81,6 +81,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.EnumMap;
+import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -205,9 +206,20 @@ public abstract class TcpTransport<Channel> extends AbstractLifecycleComponent i
         ConnectionProfile.Builder builder = new ConnectionProfile.Builder();
         builder.addConnections(connectionsPerNodeBulk, TransportRequestOptions.Type.BULK);
         builder.addConnections(connectionsPerNodePing, TransportRequestOptions.Type.PING);
-        builder.addConnections(connectionsPerNodeRecovery, TransportRequestOptions.Type.RECOVERY);
-        builder.addConnections(connectionsPerNodeReg, TransportRequestOptions.Type.REG);
-        builder.addConnections(connectionsPerNodeState, TransportRequestOptions.Type.STATE);
+        Set<TransportRequestOptions.Type> regSet = EnumSet.of(TransportRequestOptions.Type.REG);
+        if (DiscoveryNode.isMasterNode(settings)) {
+            builder.addConnections(connectionsPerNodeState, TransportRequestOptions.Type.STATE);
+        } else {
+            // if we are not master eligible we don't need a dedicated channel to publish the state
+            regSet.add(TransportRequestOptions.Type.STATE);
+        }
+        if (DiscoveryNode.isDataNode(settings)) {
+            builder.addConnections(connectionsPerNodeRecovery, TransportRequestOptions.Type.RECOVERY);
+        } else {
+            // if we are not a data-node we don't need any dedicated channels for recovery
+            regSet.add(TransportRequestOptions.Type.RECOVERY);
+        }
+        builder.addConnections(connectionsPerNodeReg, regSet.toArray(new TransportRequestOptions.Type[regSet.size()]));
         defaultConnectionProfile = builder.build();
     }

jasontedor

I like it @s1monw. I left a few comments.

jasontedor · 2016-11-28T14:48:44Z

core/src/main/java/org/elasticsearch/transport/TcpTransport.java

-     * @see #connectToChannelsLight(DiscoveryNode)
-     */
-    protected abstract NodeChannels connectToChannels(DiscoveryNode node) throws IOException;
+    protected abstract NodeChannels connectToChannels(DiscoveryNode node, ConnectionProfile handles) throws IOException;


Nit: handles -> connectionProfile.

jasontedor · 2016-11-28T14:54:36Z

core/src/main/java/org/elasticsearch/transport/ConnectionProfile.java

+            TransportRequestOptions.Type.STATE)), 1);
+
+    private final List<ConnectionTypeHandle> handles;
+    private final int numConnection;


Nit: numConnection -> numConnections.

jasontedor · 2016-11-28T14:54:54Z

core/src/main/java/org/elasticsearch/transport/ConnectionProfile.java

+    /**
+     * Returns the total number of connections for this profile
+     */
+    public int getNumConnection() {


Nit: getNumConnection -> getNumConnections.

jasontedor · 2016-11-28T16:05:45Z

core/src/main/java/org/elasticsearch/transport/ConnectionProfile.java

+            }
+            for (TransportRequestOptions.Type type : types) {
+                if (addedTypes.contains(type)) {
+                    throw new IllegalArgumentException("type [" + type + "] is already registered");


I think that this should be an IllegalStateException.

my reasoning is that the argument would introduce this state but I won't let it hence the ISE

jasontedor · 2016-11-28T16:06:24Z

core/src/main/java/org/elasticsearch/transport/ConnectionProfile.java

+            offset += numConnections;
+        }
+
+        public ConnectionProfile build() {


Can you add a Javadoc for this documenting that all types have to have been added?

s1monw · 2016-11-28T16:46:36Z

@jasontedor I pushed a new commit and replied to one comment

jasontedor

I left one more comment @s1monw.

jasontedor · 2016-11-28T17:27:10Z

core/src/main/java/org/elasticsearch/transport/ConnectionProfile.java

@@ -79,6 +79,10 @@ public void addConnections(int numConnections, TransportRequestOptions.Type... t
            offset += numConnections;
        }

+        /**
+         * Creates a new {@link ConnectionProfile} based on the added connections.
+         * @throws IllegalArgumentException if any of the {@link org.elasticsearch.transport.TransportRequestOptions.Type} enum is missing


This Javadoc is incorrect; the method throws an IllegalStateException (which in this case I think is correct). 😄

s1monw · 2016-11-28T20:58:51Z

@jasontedor fixed the javadocs issue

jasontedor

LGTM.

…21799) The Transport#connectToNodeLight concepts is confusing and not very flexible. neither really testable on a unittest level. This commit cleans up the code used to connect to nodes and simplifies transport implementations to share more code. This also allows to connect to nodes with custom profiles if needed, for instance future improvements can be added to connect to/from nodes that are non-data nodes without dedicated bulks and recovery connections.

s1monw added :Distributed Coordination/Network Http and internode communication implementations >enhancement review v5.2.0 v6.0.0-alpha1 labels Nov 25, 2016

tlrx reviewed Nov 25, 2016

View reviewed changes

s1monw added 3 commits November 27, 2016 09:00

add more missing javadocs

aa8cf7d

fix checkstyle issues

e9fb8f4

fix TCPTransportTests

9930b71

jasontedor requested changes Nov 28, 2016

View reviewed changes

review comments

c03313c

jasontedor reviewed Nov 28, 2016

View reviewed changes

Fix javadoc

adb2007

jasontedor approved these changes Nov 28, 2016

View reviewed changes

s1monw merged commit f5ff69f into elastic:master Nov 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove connectToNodeLight and replace it with a connection profile #21799

Remove connectToNodeLight and replace it with a connection profile #21799

s1monw commented Nov 25, 2016

s1monw commented Nov 25, 2016

tlrx Nov 25, 2016

s1monw Nov 25, 2016

tlrx Nov 25, 2016

s1monw Nov 27, 2016

s1monw Nov 27, 2016

tlrx Nov 28, 2016

s1monw Nov 28, 2016

tlrx Nov 28, 2016

s1monw commented Nov 27, 2016

s1monw commented Nov 28, 2016

jasontedor left a comment

jasontedor Nov 28, 2016

jasontedor Nov 28, 2016

jasontedor Nov 28, 2016

jasontedor Nov 28, 2016

s1monw Nov 28, 2016

jasontedor Nov 28, 2016

jasontedor Nov 28, 2016

s1monw commented Nov 28, 2016

jasontedor left a comment

jasontedor Nov 28, 2016

s1monw commented Nov 28, 2016

jasontedor left a comment

Remove connectToNodeLight and replace it with a connection profile #21799

Remove connectToNodeLight and replace it with a connection profile #21799

Conversation

s1monw commented Nov 25, 2016

s1monw commented Nov 25, 2016

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

s1monw commented Nov 27, 2016

s1monw commented Nov 28, 2016

jasontedor 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

s1monw commented Nov 28, 2016

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw commented Nov 28, 2016

jasontedor left a comment

Choose a reason for hiding this comment