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/08/26 13:55:36 UTC
[GitHub] [tinkerpop] OGoodness opened a new pull request, #1792: Implement Python Query/Connection Retry Logic
OGoodness opened a new pull request, #1792:
URL: https://github.com/apache/tinkerpop/pull/1792
Changes:
- `Dockerfile.template` updated maven version due to referenced version no longer being available from main apache download.
- CDN Link that no longer works: https://dlcdn.apache.org/maven/maven-3/3.8.5/binaries/
- Archive CDN Link: https://archive.apache.org/dist/maven/maven-3/3.8.5/binaries/
- New Version: https://dlcdn.apache.org/maven/maven-3/3.8.6/binaries/
- Updated `driver_remote_connection.py` to attempt retry logic if specified by user during request.
- This is to replace a common pattern where the end user is forced to implement their own retry logic which ends up being in the form of a decorator on every method that performs a query. Allowing the user to attempt a retry here will avoid the need for manual implementation.
--
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] spmallette commented on pull request #1792: Implement Python Query/Connection Retry Logic
Posted by GitBox <gi...@apache.org>.
spmallette commented on PR #1792:
URL: https://github.com/apache/tinkerpop/pull/1792#issuecomment-1231486817
I have a few general points to offer related to "retry" being built into the drivers. First, the community has typically shied away from this feature leaving it to users to determine when the retry should occur. The last discussion I can recall on this topic took place before 3.5.x when a specific retry error code was introduced which meant that servers could specifically let the client know when retry made sense. I don't know if that would change the original thinking. Second, in recent years we've been striving for parity among the drivers, so adding retry to python would entail a larger discussion on adding it for all the drivers. Let's see what others have to say on the topic.
--
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] spmallette commented on pull request #1792: Implement Python Query/Connection Retry Logic
Posted by GitBox <gi...@apache.org>.
spmallette commented on PR #1792:
URL: https://github.com/apache/tinkerpop/pull/1792#issuecomment-1228537486
thanks for your contribution. as an initial point regarding the docker/maven CDN issues, could you please form that as a separate PR to the `3.5-dev` branch? i think that rather than bumping the version you should instead modify the download url to go to the Apache archives rather than change the version. The Apache archives are here:
https://archive.apache.org/dist/maven/maven-3/3.8.5/
and i think you would just need to modify this shell script here:
https://github.com/apache/tinkerpop/blob/master/.travis.install-maven.sh
--
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] OGoodness commented on pull request #1792: Implement Python Query/Connection Retry Logic
Posted by GitBox <gi...@apache.org>.
OGoodness commented on PR #1792:
URL: https://github.com/apache/tinkerpop/pull/1792#issuecomment-1228602374
> thanks for your contribution. as an initial point regarding the docker/maven CDN issues, could you please form that as a separate PR to the `3.5-dev` branch? i think that rather than bumping the version you should instead modify the download url to go to the Apache archives rather than change the version. The Apache archives are here:
>
> https://archive.apache.org/dist/maven/maven-3/3.8.5/
>
> and i think you would just need to modify this shell script here:
>
> https://github.com/apache/tinkerpop/blob/master/.travis.install-maven.sh
Definitely! Changing the url to the Archive endpoint was actually my initial solution, but I was unsure how committed the service was to keeping with more modern maven versions, so I just bumped it.
I'll create a new CR with the archive CDN later today and remove it from this one.
--
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] spmallette commented on pull request #1792: Implement Python Query/Connection Retry Logic
Posted by GitBox <gi...@apache.org>.
spmallette commented on PR #1792:
URL: https://github.com/apache/tinkerpop/pull/1792#issuecomment-1371397442
closing this for now since there hasn't bee much activity. please feel free to bring the discussion up again should you want to come back to this.
--
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] spmallette commented on pull request #1792: Implement Python Query/Connection Retry Logic
Posted by GitBox <gi...@apache.org>.
spmallette commented on PR #1792:
URL: https://github.com/apache/tinkerpop/pull/1792#issuecomment-1244253241
> Is this due to complexity or because the feature is not desirable?
Just to clarify, as the feature was originally proposed it was, at the time, found not desirable. That wasn't to say that if it were re-proposed now under new circumstances that no one would want it. I'd also keep in mind that the last time this was discussed to my recollection was several years ago, prior to the introduction of the specific retry error code, so I was just trying to provide historical context.
I think the more important take here is feature parity in the drivers - if we do it for one, it should be done for all and any PR (or series of PRs) would need to have a plan for that. The further implication then is that that sort of makes this a major feature and requires a wide community consensus. Usually a feature with this sort of scope needs more than just a PR to get that consensus - it would require a DISCUSS thread on the dev mailing list with some sort of proposal for what the design would be. We've already had some discussion here on it so summarizing all this into a post and perhaps addressing some of the points made there would be a good start.
--
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] kenhuuu commented on pull request #1792: Implement Python Query/Connection Retry Logic
Posted by GitBox <gi...@apache.org>.
kenhuuu commented on PR #1792:
URL: https://github.com/apache/tinkerpop/pull/1792#issuecomment-1233191336
Hi, thanks for contributing. I have a few questions/comments about this PR.
1. It seems like the retry logic may have been missed for submit_async().
2. Maybe we should consider adding exponential backoff and a maximum number of retries?
3. A possible improvement might be to test if the error should be retried. E.g. don't retry for non-recoverable errors like incorrect password.
4. It may make more sense to handle the connection pool logic in the client.
5. Some added documentation for this feature would probably be useful for others.
--
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] OGoodness commented on pull request #1792: Implement Python Query/Connection Retry Logic
Posted by GitBox <gi...@apache.org>.
OGoodness commented on PR #1792:
URL: https://github.com/apache/tinkerpop/pull/1792#issuecomment-1245590655
@spmallette Gotcha, thank you for that! Just wanted a better understanding of the situation.
When I have time, I'll go back and update this PR to address the comments here, as I agree with the concerns.
Thank you all for considering this!
Once the approach for Python is more ironed out, I suppose we can use that as a chance to discuss whether it should be implemented in the other versions and thus making it a major feature.
--
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] FlorianHockmann commented on pull request #1792: Implement Python Query/Connection Retry Logic
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on PR #1792:
URL: https://github.com/apache/tinkerpop/pull/1792#issuecomment-1239127692
I agree that adding retry logic directly into the drivers isn't trivial. @kenhuuu mentions an important topic that we have to consider if we want to add a general retry logic:
> A possible improvement might be to test if the error should be retried. E.g. don't retry for non-recoverable errors like incorrect password.
Apart from non-recoverable errors, can't we also run into a situation where a traversal was already successfully evaluated on the server but sending back its results to the driver failed due to some network problem? Simply sending that traversal (which must be considered as failed by the driver as it didn't receive a successful response by the server) to the server again can be problematic if the traversal modified the graph, e.g., we shouldn't resend an `addV()` step as it results in duplicates.
[This article](https://devblogs.microsoft.com/azure-sql/configurable-retry-logic-for-microsoft-data-sqlclient/) explains some considerations that were made when a retry logic was added to Microsoft's .NET SQL client. That's also where I got the scenario from I just described with mutating traversals. They handle it by letting users configure which SQL commands should be retried / which not so that mutating commands can be skipped for the retry.
I think we can add a retry logic to the drivers, but we should make it configurable for users. This means that users should be able to configure:
1. Whether they want to use our retry logic in general (so they can implement their own instead / don't use a retry logic)
2. Which exceptions should be retried, e.g., transient network errors, but not failures from the server or only specific failures from the server, but not a `FORBIDDEN` response for example.
3. Which traversals should be retried, e.g., retry a `g.V().has()[...]` traversal, but not a mutating traversal.
4. Number of retries, times to wait between retry, and so on (exponential retry with / without a random jitter could be added, but doesn't have to be, especially in the first version, in my opinion).
Number 3 is probably a lot easier to implement, then letting users specify whether they don't want to retry specific Gremlin steps or mutating traversals in general.
--
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 #1792: Implement Python Query/Connection Retry Logic
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1792:
URL: https://github.com/apache/tinkerpop/pull/1792#issuecomment-1228554213
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1792?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 [#1792](https://codecov.io/gh/apache/tinkerpop/pull/1792?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (818c16d) into [master](https://codecov.io/gh/apache/tinkerpop/commit/da3474559e0213a5a37e7ba9905ac5cd957a5d23?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (da34745) will **decrease** coverage by `5.14%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #1792 +/- ##
============================================
- Coverage 69.21% 64.06% -5.15%
============================================
Files 875 23 -852
Lines 41744 3679 -38065
Branches 5622 0 -5622
============================================
- Hits 28893 2357 -26536
+ Misses 10875 1154 -9721
+ Partials 1976 168 -1808
```
| [Impacted Files](https://codecov.io/gh/apache/tinkerpop/pull/1792?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...mlin/structure/util/keyed/KeyedVertexProperty.java](https://codecov.io/gh/apache/tinkerpop/pull/1792/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvdXRpbC9rZXllZC9LZXllZFZlcnRleFByb3BlcnR5LmphdmE=) | | |
| [...mlin/process/traversal/step/map/SumGlobalStep.java](https://codecov.io/gh/apache/tinkerpop/pull/1792/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9zdGVwL21hcC9TdW1HbG9iYWxTdGVwLmphdmE=) | | |
| [...rocess/traversal/traverser/ProjectedTraverser.java](https://codecov.io/gh/apache/tinkerpop/pull/1792/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC90cmF2ZXJzZXIvUHJvamVjdGVkVHJhdmVyc2VyLmphdmE=) | | |
| [...e/tinkerpop/gremlin/util/function/TriFunction.java](https://codecov.io/gh/apache/tinkerpop/pull/1792/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi91dGlsL2Z1bmN0aW9uL1RyaUZ1bmN0aW9uLmphdmE=) | | |
| [.../traversal/strategy/decoration/RemoteStrategy.java](https://codecov.io/gh/apache/tinkerpop/pull/1792/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3JlbW90ZS90cmF2ZXJzYWwvc3RyYXRlZ3kvZGVjb3JhdGlvbi9SZW1vdGVTdHJhdGVneS5qYXZh) | | |
| [...kerpop/gremlin/server/handler/HttpHandlerUtil.java](https://codecov.io/gh/apache/tinkerpop/pull/1792/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3RpbmtlcnBvcC9ncmVtbGluL3NlcnZlci9oYW5kbGVyL0h0dHBIYW5kbGVyVXRpbC5qYXZh) | | |
| [...ss/computer/util/AbstractVertexProgramBuilder.java](https://codecov.io/gh/apache/tinkerpop/pull/1792/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL2NvbXB1dGVyL3V0aWwvQWJzdHJhY3RWZXJ0ZXhQcm9ncmFtQnVpbGRlci5qYXZh) | | |
| [...mlin/process/traversal/step/map/ScalarMapStep.java](https://codecov.io/gh/apache/tinkerpop/pull/1792/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9zdGVwL21hcC9TY2FsYXJNYXBTdGVwLmphdmE=) | | |
| [...op/gremlin/structure/io/util/IoRegistryHelper.java](https://codecov.io/gh/apache/tinkerpop/pull/1792/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvaW8vdXRpbC9Jb1JlZ2lzdHJ5SGVscGVyLmphdmE=) | | |
| [...nkerpop/gremlin/util/function/BulkSetSupplier.java](https://codecov.io/gh/apache/tinkerpop/pull/1792/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi91dGlsL2Z1bmN0aW9uL0J1bGtTZXRTdXBwbGllci5qYXZh) | | |
| ... and [842 more](https://codecov.io/gh/apache/tinkerpop/pull/1792/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] OGoodness commented on pull request #1792: Implement Python Query/Connection Retry Logic
Posted by GitBox <gi...@apache.org>.
OGoodness commented on PR #1792:
URL: https://github.com/apache/tinkerpop/pull/1792#issuecomment-1242576586
@spmallette @FlorianHockmann @kenhuuu
## Question to the team.
Based on @spmallette 's comment regarding this feature, the team has avoided trying to implement retries. Is this due to complexity or because the feature is not desirable? If I were to address all these concerns, would it be likely for the PR to be merged in? Want to ensure this is a feature that the team is open to before fleshing out the implementation, if so, I'm more than happy to contribute :D.
---
You both bring up great points. Agreed that the current implementation could result in unintended retries, perhaps a custom error that is raised only on retriable requests would be better?
@kenhuuu
- You mentioned moving some of the logic to the client. I agree, however, my original attempts at this were trying just that. But when you get any deeper than the Driver it can get messy with async code, which I wanted to avoid.
- You mention a missed `submit_async`, you're right. If we decide to move forward with this feature I'd be more than happy to add support for it.
@FlorianHockmann
- Your concern about retrying mutations, this is similar to what I've fixed on some of my personal implementations, but had not implemented into this CR, as @spmallette suggested that this feature was unlikely to pick up. Definitely understand we need to use a lighter touch to our retry logic.
--
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] spmallette closed pull request #1792: Implement Python Query/Connection Retry Logic
Posted by GitBox <gi...@apache.org>.
spmallette closed pull request #1792: Implement Python Query/Connection Retry Logic
URL: https://github.com/apache/tinkerpop/pull/1792
--
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