[jira] [Commented] (TINKERPOP-2617) Refactor Java Driver to have one method for connection selection
[
https://issues.apache.org/jira/browse/TINKERPOP-2617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17842775#comment-17842775
]
ASF GitHub Bot commented on TINKERPOP-2617:
---
lyndonbauto commented on PR #1709:
URL: https://github.com/apache/tinkerpop/pull/1709#issuecomment-2089121052
@DKZed Where are you at with these changes?
> 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.10#820010)
[jira] [Commented] (TINKERPOP-2617) Refactor Java Driver to have one method for connection selection
[
https://issues.apache.org/jira/browse/TINKERPOP-2617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17769800#comment-17769800
]
ASF GitHub Bot commented on TINKERPOP-2617:
---
xiazcy closed pull request #1709: TINKERPOP-2617 Clean up Java connection pool
logic
URL: https://github.com/apache/tinkerpop/pull/1709
> 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.10#820010)
[jira] [Commented] (TINKERPOP-2617) Refactor Java Driver to have one method for connection selection
[
https://issues.apache.org/jira/browse/TINKERPOP-2617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17769799#comment-17769799
]
ASF GitHub Bot commented on TINKERPOP-2617:
---
xiazcy commented on PR #1709:
URL: https://github.com/apache/tinkerpop/pull/1709#issuecomment-1738061207
Been open for a while with not much activity, we are closing this PR.
> 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.10#820010)
[jira] [Commented] (TINKERPOP-2617) Refactor Java Driver to have one method for connection selection
[
https://issues.apache.org/jira/browse/TINKERPOP-2617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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
>
[jira] [Commented] (TINKERPOP-2617) Refactor Java Driver to have one method for connection selection
[
https://issues.apache.org/jira/browse/TINKERPOP-2617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17557598#comment-17557598
]
ASF GitHub Bot commented on TINKERPOP-2617:
---
simonz-bq commented on PR #1709:
URL: https://github.com/apache/tinkerpop/pull/1709#issuecomment-1163431400
> Let me get to a first review by end of this week @simonz-bq . Meanwhile,
it would be great if we could get some testing around the edge cases that you
mentioned which aren't correctly handled in the current code.
@divijvaidya the additional edge cases logic was removed, but I forgot to
update the initial commit comment. It was causing existing integration tests to
fail and hang and investigation into how to fixing them proved to be difficult
and complex for what was being considered as a easy win.
> 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)
[jira] [Commented] (TINKERPOP-2617) Refactor Java Driver to have one method for connection selection
[
https://issues.apache.org/jira/browse/TINKERPOP-2617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17557573#comment-17557573
]
ASF GitHub Bot commented on TINKERPOP-2617:
---
divijvaidya commented on PR #1709:
URL: https://github.com/apache/tinkerpop/pull/1709#issuecomment-1163392684
Let me get to a first review by end of this week @simonz-bq . Meanwhile, it
would be great if we could get some testing around the edge cases that you
mentioned which aren't correctly handled in the current code.
> 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)
[jira] [Commented] (TINKERPOP-2617) Refactor Java Driver to have one method for connection selection
[
https://issues.apache.org/jira/browse/TINKERPOP-2617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17556589#comment-17556589
]
ASF GitHub Bot commented on TINKERPOP-2617:
---
lyndonbauto commented on PR #1709:
URL: https://github.com/apache/tinkerpop/pull/1709#issuecomment-1160994147
@divijvaidya do you have time to look at this? I think you have the most
context on Java connection pooling.
> 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)
[jira] [Commented] (TINKERPOP-2617) Refactor Java Driver to have one method for connection selection
[
https://issues.apache.org/jira/browse/TINKERPOP-2617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17555775#comment-17555775
]
ASF GitHub Bot commented on TINKERPOP-2617:
---
codecov-commenter commented on PR #1709:
URL: https://github.com/apache/tinkerpop/pull/1709#issuecomment-1159270860
#
[Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1709?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation)
Report
> Merging
[#1709](https://codecov.io/gh/apache/tinkerpop/pull/1709?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation)
(6698fe8) into
[3.5-dev](https://codecov.io/gh/apache/tinkerpop/commit/3d106e8e65568133197dadcf2dd1c1736416d44a?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation)
(3d106e8) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff@@
## 3.5-dev#1709 +/- ##
Coverage63.27% 63.27%
Files 23 23
Lines 3553 3553
Hits 2248 2248
Misses1131 1131
Partials 174 174
```
--
[Continue to review full report at
Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1709?src=pr=continue_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn
more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation)
> `Δ = absolute (impact)`, `ø = not affected`, `? = missing data`
> Powered by
[Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1709?src=pr=footer_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation).
Last update
[3d106e8...6698fe8](https://codecov.io/gh/apache/tinkerpop/pull/1709?src=pr=lastupdated_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation).
Read the [comment
docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation).
> 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)
[jira] [Commented] (TINKERPOP-2617) Refactor Java Driver to have one method for connection selection
[
https://issues.apache.org/jira/browse/TINKERPOP-2617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17554869#comment-17554869
]
ASF GitHub Bot commented on TINKERPOP-2617:
---
simonz-bq opened a new pull request, #1709:
URL: https://github.com/apache/tinkerpop/pull/1709
Removed duplication while fetching a connection form the ConnectionPool.
Also, handled some potential edge cases where a user specified a minimum
connections size of 0, where there could potentially be no connections ever in
the pool to use as well as a unlikely scenario where the host would be deemed
dead despite a new connection being possible.
> 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)
