andreachild commented on code in PR #3139:
URL: https://github.com/apache/tinkerpop/pull/3139#discussion_r2159580880
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java:
##########
@@ -543,9 +543,20 @@ protected void initializeImplementation() {
this.initializationFailure = ex;
}
- // throw an error if there is no host available after initializing
connection pool.
- if (cluster.availableHosts().isEmpty())
+ // throw an error if there is no host available after initializing
connection pool. we used
+ // to test cluster.availableHosts().isEmpty() but checking if we
actually have hosts in
+ // the connection pool seems a bit more fireproof. if we look at
initializeConnectionSetupForHost
+ // we can see that a successful initialization of the
host/connection pool pair is followed by
+ // marking the host available and notifying the load balancing
strategy. by relying directly on
+ // the state of hostConnectionPools we ensure that there is
actually a concrete
+ // host/connection pool pair. even if the connection pool has
immediate problems, it can fallback
+ // to its normal reconnection operation and won't put
chooseConnection in a state where it can
+ // get a NPE if hostConnectionPools ends up being empty. it seems
as if the safest minimum
+ // requirement for leaving this method is to ensure that at least
one ConnectionPool constructor
+ // completed for at least one Host.
+ if (hostConnectionPools.isEmpty()) {
Review Comment:
This change seems OK but I am wondering if there is a deeper issue in that
`hostConnectionPools.isEmpty()` has somehow become inconsistent with
`cluster.availableHosts().isEmpty()`?
--
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]
