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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to