[
https://issues.apache.org/jira/browse/TINKERPOP-2617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17559096#comment-17559096
]
ASF GitHub Bot commented on TINKERPOP-2617:
-------------------------------------------
divijvaidya commented on code in PR #1709:
URL: https://github.com/apache/tinkerpop/pull/1709#discussion_r907169276
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java:
##########
@@ -382,35 +344,49 @@ private Connection waitForConnection(final long timeout,
final TimeUnit unit) th
long start = System.nanoTime();
long remaining = timeout;
long to = timeout;
- do {
- try {
- awaitAvailableConnection(remaining, unit);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- to = 0;
+
+ Connection leastUsed = selectLeastUsed();
+ if (leastUsed != null) {
+ final int currentPoolSize = connections.size();
+ if (leastUsed.borrowed.get() >= maxSimultaneousUsagePerConnection
&& currentPoolSize < maxPoolSize) {
+ if (logger.isDebugEnabled())
+ logger.debug("Least used {} on {} exceeds
maxSimultaneousUsagePerConnection but pool size {} < maxPoolSize - consider new
connection",
+ leastUsed.getConnectionInfo(), host,
currentPoolSize);
+ considerNewConnection();
}
+ }
+ do {
if (isClosed())
throw new ConnectionException(host.getHostUri(),
host.getAddress(), "Pool is shutdown");
- final Connection leastUsed = selectLeastUsed();
if (leastUsed != null) {
while (true) {
final int inFlight = leastUsed.borrowed.get();
final int availableInProcess =
leastUsed.availableInProcess();
- if (inFlight >= availableInProcess) {
- logger.debug("Least used {} on {} has requests
borrowed [{}] >= availableInProcess [{}] - may timeout waiting for connection",
- leastUsed, host, inFlight, availableInProcess);
+ if (inFlight >= maxSimultaneousUsagePerConnection &&
availableInProcess == 0) {
Review Comment:
We currently have a conditional check for whether the selected connection
can accept this new request at two places at line 351 and line 367. But the
current code has different conditions for checking the same logic condition. We
should unify that.
This conditional should be abstracted into a function or a functional
interface, let's say, `canAcceptNewRequestsOnConnection()` and re-use the same
conditional check on line 351
`if (!canAcceptNewRequestsOnConnection() && currentPoolSize < maxPoolSize)`.
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java:
##########
@@ -382,35 +344,49 @@ private Connection waitForConnection(final long timeout,
final TimeUnit unit) th
long start = System.nanoTime();
long remaining = timeout;
long to = timeout;
- do {
- try {
- awaitAvailableConnection(remaining, unit);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- to = 0;
+
+ Connection leastUsed = selectLeastUsed();
Review Comment:
Perhaps refactor as following for returning the happy case early. It helps
in readability since the reader now understands that the code after this line
is handling logic for case when no connection is immediately available.
You can do something like:
```
// scenario 1: when connection pool is empty
if (connections.empty()) {
for (size 1..minPoolSize)
createNewConnection()
return waitForConnection()
}
// scenario 2: happy case when a connection is available
Connection leastUsed = selectLeastUsed();
if (leastUsed != null && leastUsed.canAcceptNewRequests)
return leastUsed
// scenario 3: wait for a connection to become available
maybeCreateNewConnection() // checks if it is possible to create a new
connection, if yes, creates one.
return waitForConnection() // doesn't need to consider the older leastUsed,
it will wait first and then ask another.
fun waitForConnection {
do {
awaitForAvailableConnection
find least used
if (leastUsed.canAcceptNewRequests)
return leastUsed
} while (timeout pending)
}
```
> Refactor Java Driver to have one method for connection selection
> ----------------------------------------------------------------
>
> Key: TINKERPOP-2617
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2617
> Project: TinkerPop
> Issue Type: Improvement
> Components: driver
> Affects Versions: 3.4.12
> Reporter: Stephen Mallette
> Priority: Minor
>
> To make a decision on whether a connection should be borrowed or not, a Java
> client today does two logic which are different from each other. They should
> both be same. One bit of logic to do so is at:
> {code}
> if (borrowed >= maxSimultaneousUsagePerConnection &&
> leastUsedConn.availableInProcess() == 0) {
> {code}
> and another is:
> {code}
> final int inFlight = leastUsed.borrowed.get();
> final int availableInProcess =
> leastUsed.availableInProcess();
> if (inFlight >= availableInProcess) {
> logger.debug("Least used {} on {} has requests
> borrowed [{}] >= availableInProcess [{}] - may timeout waiting for
> connection",
> leastUsed, host, inFlight,
> availableInProcess);
> break;
> }
> if (leastUsed.borrowed.compareAndSet(inFlight, inFlight +
> 1)) {
> if (logger.isDebugEnabled())
> logger.debug("Return least used {} on {} after
> waiting", leastUsed.getConnectionInfo(), host);
> return leastUsed;
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
