You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2021/09/09 17:07:00 UTC
[jira] [Commented] (TINKERPOP-2486) Client does not load balance
requests across available connections
[ https://issues.apache.org/jira/browse/TINKERPOP-2486?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17412710#comment-17412710 ]
ASF GitHub Bot commented on TINKERPOP-2486:
-------------------------------------------
divijvaidya commented on a change in pull request #1465:
URL: https://github.com/apache/tinkerpop/pull/1465#discussion_r705292510
##########
File path: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java
##########
@@ -158,38 +156,40 @@ public Connection borrowConnection(final long timeout, final TimeUnit unit) thro
return waitForConnection(timeout, unit);
}
+ final Connection leastUsedConn = selectLeastUsed();
+
if (null == leastUsedConn) {
if (isClosed())
throw new ConnectionException(host.getHostUri(), host.getAddress(), "Pool is shutdown");
logger.debug("Pool was initialized but a connection could not be selected earlier - waiting for connection on {}", host);
return waitForConnection(timeout, unit);
}
+ // Currently borrowed and used connections is 1 less than borrowed which is incremented by selectLeastUsed
+ final int borrowedInUse = leastUsedConn.borrowed.get() - 1;
+
// if the number borrowed on the least used connection exceeds the max allowed and the pool size is
// not at maximum then consider opening a connection
final int currentPoolSize = connections.size();
- if (leastUsedConn.borrowed.get() >= maxSimultaneousUsagePerConnection && currentPoolSize < maxPoolSize) {
+ if (borrowedInUse >= maxSimultaneousUsagePerConnection && currentPoolSize < maxPoolSize) {
Review comment:
This if block logic enforces that we create a new connection if the least used connection itself cannot accomodate new requests.
But consider the following scenario, we have 12 requests arriving at the same time with maxSimultaneousUsagePerConnection set to 10. Assume we have a single connection. In that case, borrowed for leastUsedConnection will be set to 12. When the first request arrives at this code after the above, it will enter the block and create a new connection, similarly second will also enter this code block.
We will end up creating/requesting 12 connections. However, we just wanted to create 1 additional connection which can accomodate the extra 2 requests.
Please also add a test case which should fail for above scenario.
##########
File path: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java
##########
@@ -158,38 +156,40 @@ public Connection borrowConnection(final long timeout, final TimeUnit unit) thro
return waitForConnection(timeout, unit);
}
+ final Connection leastUsedConn = selectLeastUsed();
Review comment:
Please add a comment explaining the new logic. The comment would explain the following in some detail: "we eagerly borrow the the least used connection and then perform validation if we can actually use it or not. If validations fail, we lend (un-borrow?) the connection back and we wait for a new connection to become available."
##########
File path: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java
##########
@@ -475,7 +474,7 @@ private void announceAvailableConnection() {
}
}
- private Connection selectLeastUsed() {
+ private synchronized Connection selectLeastUsed() {
Review comment:
With your change, this function does more than just selecting the least used connection. It also borrows it. Please rename the function to borrowLeastUsedConnection()
--
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
> Client does not load balance requests across available connections
> ------------------------------------------------------------------
>
> Key: TINKERPOP-2486
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2486
> Project: TinkerPop
> Issue Type: Bug
> Components: driver
> Affects Versions: 3.4.8, 3.4.9
> Reporter: Divij Vaidya
> Priority: Major
>
> The client does not load balance requests across connections in a threadpool which cause a request failing with timeout even when a connection is available. To verify this, the following test fails:
> {code:java}
> @Test
> public void shouldBalanceConcurrentRequestsAcrossConnections() throws InterruptedException {
> final int connPoolSize = 16;
> final Cluster cluster = TestClientFactory.build()
> .minConnectionPoolSize(connPoolSize)
> .maxConnectionPoolSize(connPoolSize)
> .create();
> final Client.ClusteredClient client = cluster.connect();
> client.init();
> try {
> final RequestMessage.Builder request = client.buildMessage(RequestMessage.build(Tokens.OPS_EVAL))
> .add(Tokens.ARGS_GREMLIN, "Thread.sleep(5000)");
> final Callable<Connection> sendQueryCallable = () -> client.chooseConnection(request.create());
> final List<Callable<Connection>> listOfTasks = new ArrayList<>();
> for (int i=0; i<connPoolSize; i++) {
> listOfTasks.add(sendQueryCallable);
> }
> Set<String> channels = new HashSet<>();
> final List<Future<Connection>> executorSubmitFutures = executorServiceForTesting.invokeAll(listOfTasks);
> executorSubmitFutures.parallelStream().map(fut -> {
> try {
> return fut.get();
> } catch (InterruptedException e) {
> e.printStackTrace();
> } catch (ExecutionException e) {
> e.printStackTrace();
> }
> return null;
> }).forEach(conn -> channels.add(conn.getChannelId()));
>
> System.out.println(channels.size());
> } finally {
> cluster.close();
> }
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)