[
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
> 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)
