Cole-Greer commented on code in PR #3139:
URL: https://github.com/apache/tinkerpop/pull/3139#discussion_r2159603894
##########
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:
From what I gather, there is no difference here if there is only 1 client
per cluster as the connection is marked available in the cluster at the same
time that the connection pool is added to `hostConnectionPools` for the client:
https://github.com/apache/tinkerpop/blob/3a48401d83656a31c459f541550b62be67084970/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java#L589C1-L592C38
If this change is fixing some NPE error, to me that's implying that
something in the ConnectionPool constructor is failing to mark a host as
unavailable if the initialization was unsuccessful.
--
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]
