You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2021/05/19 18:40:21 UTC
[GitHub] [tinkerpop] spmallette opened a new pull request #1422: TINKERPOP-2569 Reconnect if java driver fails to initialize
spmallette opened a new pull request #1422:
URL: https://github.com/apache/tinkerpop/pull/1422
https://issues.apache.org/jira/browse/TINKERPOP-2569
Does result in a slight change in behavior as client initialization is now aware of dead hosts and will throw a `NoHostAvailableException` earlier (used to wait for a request to be submitted). This is probably better behavior. It seems unlikely that anyone would rely on this behavior for normal usage so pushing it to 3.4.x seems safe.
Builds with `mvn clean install && mvn verify -pl gremlin-server -DskipIntegrationTests=false`
VOTE +1
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette closed pull request #1422: TINKERPOP-2569 Reconnect if java driver fails to initialize
Posted by GitBox <gi...@apache.org>.
spmallette closed pull request #1422:
URL: https://github.com/apache/tinkerpop/pull/1422
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1422: TINKERPOP-2569 Reconnect if java driver fails to initialize
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1422:
URL: https://github.com/apache/tinkerpop/pull/1422#discussion_r642584008
##########
File path: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java
##########
@@ -111,8 +111,11 @@ public ConnectionPool(final Host host, final Client client, final Optional<Integ
logger.warn("Initialization of connections cancelled for {}", getPoolInfo(), ce);
throw ce;
} catch (CompletionException ce) {
- // Some connections might have been initialized. Close the connection pool gracefully to close them.
- this.closeAsync();
+ // since there was some error here, the host is likely unavailable at this point. if we dont mark it as
+ // such the client won't try to reconnect to the dead host. need to initialize this.open because if we
+ // don't then reconnects will fail when trying to create a new one with addConnectionIfUnderMaximum()
+ this.open = new AtomicInteger(0);
+ considerHostUnavailable();
Review comment:
This was intentionally removed in https://github.com/apache/tinkerpop/pull/1360
Here's what happens if we don't remove this:
The connection has failed here. It could be due to misconfiguration such as wrong ssl credentials. `considerHostUnavailable()` will create a background task which will try to send reconnect message to the host. To send that message, it will try to borrowConnection from connection pool. But connection pool had a problem during initialization in the first place. It will try to initialise connections again. It will fail again due to ssl misconfiguration. On failure, it will call `considerHostUnavailable()` again. After some wait, borrowConnection will try to create connections again which will again fail and eventually after a timeout everything would give up and throw timeout back to application. On application side, they will observe TimedOut exception and have no clue why the connection failed because the actual reason of failure is hidden in logs from 15 min ago.
Another example of complication, on `considerHostUnavailable()` we create a new connection for reconnect message asynchronously but right after we also destroy all existing connections. In some race condition, we could end up in situation where we are adding connection for reconnect and then destroying them due to earlier logic.
IMO, to keep things simple, if a client is not able to connect to the server on initialisation, it should fail fast with an exception and let application layer handle the scenario. The application could choose to recreate the client at that stage or fail their application startup and check the logs for configuration errors.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1422: TINKERPOP-2569 Reconnect if java driver fails to initialize
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1422:
URL: https://github.com/apache/tinkerpop/pull/1422#discussion_r643116539
##########
File path: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java
##########
@@ -111,8 +111,11 @@ public ConnectionPool(final Host host, final Client client, final Optional<Integ
logger.warn("Initialization of connections cancelled for {}", getPoolInfo(), ce);
throw ce;
} catch (CompletionException ce) {
- // Some connections might have been initialized. Close the connection pool gracefully to close them.
- this.closeAsync();
+ // since there was some error here, the host is likely unavailable at this point. if we dont mark it as
+ // such the client won't try to reconnect to the dead host. need to initialize this.open because if we
+ // don't then reconnects will fail when trying to create a new one with addConnectionIfUnderMaximum()
+ this.open = new AtomicInteger(0);
+ considerHostUnavailable();
Review comment:
i'd forgotten we removed that and when i was looking at the user concern i'd remembered only how it used to work. maybe i can make some aspect of this "better" in a different fashion that doesn't bring all those problems you mentioned.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org