[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #1882: TINKERPOP-2813 Make gremlin-driver more optimistic about host availability

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

vkagamlyk commented on code in PR #1882:
URL: https://github.com/apache/tinkerpop/pull/1882#discussion_r1040027645


##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java:
##########
@@ -495,49 +496,45 @@ public Client alias(final Map<String, String> aliases) {
         protected Connection chooseConnection(final RequestMessage msg) throws TimeoutException, ConnectionException {
             final Iterator<Host> possibleHosts;
             if (msg.optionalArgs(Tokens.ARGS_HOST).isPresent()) {
-                // TODO: not sure what should be done if unavailable - select new host and re-submit traversal?
+                // looking at this code about putting the Host on the RequestMessage in light of 3.5.4, not sure
+                // this is being used as intended here. server side usage is to place the channel.remoteAddress
+                // in this token in the status metadata for the response. can't remember why it is being used this
+                // way here exactly. created TINKERPOP-2821 to examine this more carefully to clean this up in a
+                // future version.
                 final Host host = (Host) msg.getArgs().get(Tokens.ARGS_HOST);
                 msg.getArgs().remove(Tokens.ARGS_HOST);
                 possibleHosts = IteratorUtils.of(host);
             } else {
                 possibleHosts = this.cluster.loadBalancingStrategy().select(msg);
             }
 
-            // you can get no possible hosts in more than a few situations. perhaps the servers are just all down.
-            // or perhaps the client is not configured properly (disables ssl when ssl is enabled on the server).
-            if (!possibleHosts.hasNext())
-                throwNoHostAvailableException();
-
-            final Host bestHost = possibleHosts.next();
+            // try a random host if none are marked available. maybe it will reconnect in the meantime. better than
+            // going straight to a fast NoHostAvailableException as was the case in versions 3.5.4 and earlier
+            final Host bestHost = possibleHosts.hasNext() ? possibleHosts.next() : chooseRandomHost();

Review Comment:
   For example we have a cluster of 3 instances.
   For some reason (reconfiguration, scaling, network fluke) all instances restarted.
   After some time can we connect to one random host and use it, but all the others will remain disconnected.
   Maybe it makes sense to check all unavailable hosts when the user sends a request, but not more than once every 10-60 seconds?



-- 
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