[GitHub] [tinkerpop] divijvaidya commented on a diff in pull request #1833: TINKERPOP-2814 Add configuration for SSL handshake timeout.

[GitBox <gi...@apache.org>]

divijvaidya commented on code in PR #1833:
URL: https://github.com/apache/tinkerpop/pull/1833#discussion_r1000533869


##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java:
##########
@@ -205,6 +205,7 @@ private static Builder getBuilderFromSettings(final Settings settings) {
                 .maxConnectionPoolSize(settings.connectionPool.maxSize)
                 .minConnectionPoolSize(settings.connectionPool.minSize)
                 .connectionSetupTimeoutMillis(settings.connectionPool.connectionSetupTimeoutMillis)
+                .sslHandshakeTimeoutMillis(settings.connectionPool.sslHandshakeTimeoutMillis)

Review Comment:
   Instead of adding yet another configuration for the users to tweak on the client, is it possible to define this as a percentage of `connectionSetupTimeoutMillis`?



##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java:
##########
@@ -986,13 +996,26 @@ public Builder port(final int port) {
          * handshake and SSL handshake. Beyond this duration an exception would be thrown.
          *
          * Note that this value should be greater that SSL handshake timeout defined in
-         * {@link io.netty.handler.ssl.SslHandler} since WebSocket handshake include SSL handshake.
+         * {@link io.netty.handler.ssl.SslHandler} since WebSocket handshake include SSL handshake. This value should be
+         * less than {@link Builder#maxWaitForConnection}.

Review Comment:
   please update comment to include `connectionSetupTimeoutMillis`



##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java:
##########
@@ -59,14 +59,15 @@ final class Connection {
 
     public static final int MAX_IN_PROCESS = 4;
     public static final int MIN_IN_PROCESS = 1;
-    public static final int MAX_WAIT_FOR_CONNECTION = 16000;
+    public static final int MAX_WAIT_FOR_CONNECTION = 25000;

Review Comment:
   Changing default could be a breaking change for users. As an example, a user may be relying on connection setup to be at 15s based on which they may have configured throttling rules in their application. 
   
   I would suggest to target the change in default values for v3.7.x



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org