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 2022/06/16 02:32:03 UTC
[GitHub] [tinkerpop] simonz-bq opened a new pull request, #1709: TINKERPOP-2617 Clean up Java connection pool logic
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.
--
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
[GitHub] [tinkerpop] codecov-commenter commented on pull request #1709: TINKERPOP-2617 Clean up Java connection pool logic
Posted by GitBox <gi...@apache.org>.
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&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1709](https://codecov.io/gh/apache/tinkerpop/pull/1709?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6698fe8) into [3.5-dev](https://codecov.io/gh/apache/tinkerpop/commit/3d106e8e65568133197dadcf2dd1c1736416d44a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3d106e8) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## 3.5-dev #1709 +/- ##
========================================
Coverage 63.27% 63.27%
========================================
Files 23 23
Lines 3553 3553
========================================
Hits 2248 2248
Misses 1131 1131
Partials 174 174
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1709?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1709?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3d106e8...6698fe8](https://codecov.io/gh/apache/tinkerpop/pull/1709?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [tinkerpop] xiazcy closed pull request #1709: TINKERPOP-2617 Clean up Java connection pool logic
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy closed pull request #1709: TINKERPOP-2617 Clean up Java connection pool logic
URL: https://github.com/apache/tinkerpop/pull/1709
--
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
[GitHub] [tinkerpop] simonz-bq commented on pull request #1709: TINKERPOP-2617 Clean up Java connection pool logic
Posted by GitBox <gi...@apache.org>.
simonz-bq commented on PR #1709:
URL: https://github.com/apache/tinkerpop/pull/1709#issuecomment-1169410810
Regarding the ask for more tests, the logic did not change. This is a refactor and the functionality behaves similar to before and is ultimately just for the sake of reducing code duplication.
Originally I had made some minor logic adjustments that were supposed to improve the logic, but it lead to nebulous failures with existing tests, and how to fix them or write new ones is very esoteric. Investigating how to fix them exceeded a time box that we'd expected for what was supposed to be a simple code refactor.
More work can definitely be done regarding the connection pooling, but I think doing so here would exceed the scope of the ticket and what we have here is sufficient for the ask at hand.
--
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
Re: [PR] TINKERPOP-2617 Clean up Java connection pool logic [tinkerpop]
Posted by "lyndonbauto (via GitHub)" <gi...@apache.org>.
lyndonbauto commented on PR #1709:
URL: https://github.com/apache/tinkerpop/pull/1709#issuecomment-2089121052
@DKZed Where are you at with these changes?
--
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
[GitHub] [tinkerpop] simonz-bq commented on pull request #1709: TINKERPOP-2617 Clean up Java connection pool logic
Posted by GitBox <gi...@apache.org>.
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.
--
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
[GitHub] [tinkerpop] lyndonbauto commented on pull request #1709: TINKERPOP-2617 Clean up Java connection pool logic
Posted by GitBox <gi...@apache.org>.
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.
--
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
[GitHub] [tinkerpop] divijvaidya commented on a diff in pull request #1709: TINKERPOP-2617 Clean up Java connection pool logic
Posted by GitBox <gi...@apache.org>.
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)
}
```
--
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
[GitHub] [tinkerpop] xiazcy commented on pull request #1709: TINKERPOP-2617 Clean up Java connection pool logic
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
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.
--
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
[GitHub] [tinkerpop] divijvaidya commented on pull request #1709: TINKERPOP-2617 Clean up Java connection pool logic
Posted by GitBox <gi...@apache.org>.
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.
--
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
[GitHub] [tinkerpop] simonz-bq commented on a diff in pull request #1709: TINKERPOP-2617 Clean up Java connection pool logic
Posted by GitBox <gi...@apache.org>.
simonz-bq commented on code in PR #1709:
URL: https://github.com/apache/tinkerpop/pull/1709#discussion_r909095165
##########
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:
I am confused as to what is being proposed here.
This is more or less the way the existing logic is written. The ticket was created with the acceptance criteria to avoid the duplication of the checking in scenario 2 and 3, where now it is combined into a single point of checking in `waitForConnection`.
Whilst I agree that it is now less readable, unfortunately this is the result.
I will add some comments to make it more clear what is happening.
--
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