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/03 20:46:38 UTC
[GitHub] [tinkerpop] xiazcy opened a new pull request, #1680: TINKERPOP-2708: unhandledRejection upon connection failure
xiazcy opened a new pull request, #1680:
URL: https://github.com/apache/tinkerpop/pull/1680
As suggested in https://issues.apache.org/jira/browse/TINKERPOP-2708, deprecated and removed the functionality of the connection option `connectOnStartup`. Since it is generally not good to return promises in constructors, removing the call to `open()` in the `Connection` constructor seemed to be the way to resolve unhandledRejection and any potential race conditions.
There is a slight API behaviour change as a result. Setting `connectOnStartup` true now will log a deprecated warning to the user. The `Connection` constructor will no longer initiate connection in the background by default, so users will need to call `open()` explicitly after creating a connection object. However, open() is called anyways when connection submits traversals, removing this shouldn't affect any normal traversal functionality.
As this may be considered a breaking change, I added a change-log entry, are there additional docs I should edit?
As a side note, this is related to https://issues.apache.org/jira/browse/TINKERPOP-2381. I believe removing connectOnStartup functionality resolves the issues listed. I'm not sure if there is a need to refactor how Client invokes `connection.open()`, since inside `connection.open()`, it does check if it has been called previously.
--
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 #1680: TINKERPOP-2708: unhandledRejection upon connection failure - API change - [breaking]
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on PR #1680:
URL: https://github.com/apache/tinkerpop/pull/1680#issuecomment-1152665604
VOTE+1
--
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 merged pull request #1680: TINKERPOP-2708: unhandledRejection upon connection failure - API change - [breaking]
Posted by GitBox <gi...@apache.org>.
lyndonbauto merged PR #1680:
URL: https://github.com/apache/tinkerpop/pull/1680
--
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 a diff in pull request #1680: TINKERPOP-2708: unhandledRejection upon connection failure
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1680:
URL: https://github.com/apache/tinkerpop/pull/1680#discussion_r891542401
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js:
##########
@@ -103,13 +103,15 @@ class Connection extends EventEmitter {
this._pingIntervalDelay = this.options.pingInterval || pingIntervalDelay;
this._pongTimeoutDelay = this.options.pongTimeout || pongTimeoutDelay;
- if (this.options.connectOnStartup !== false) {
- this.open();
+ if (this.options.connectOnStartup) {
Review Comment:
The above comment that was updated used to say the default was true, but that is not entirely correct is it? Technically it was just `!== false`?
Just want to ensure this console warning will not be printing out when a user does not set this issue.
--
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 #1680: TINKERPOP-2708: unhandledRejection upon connection failure
Posted by GitBox <gi...@apache.org>.
spmallette commented on PR #1680:
URL: https://github.com/apache/tinkerpop/pull/1680#issuecomment-1148538843
> As this may be considered a breaking change, I added a change-log entry, are there additional docs I should edit?
breaking changes are typically documented in the Upgrade Documentation so that we can call attention to important changes more easily with greater detail than the CHANGELOG - it is probably worth adding an item to say that this parameter no longer does anything and that you would want to call `open()` explicitly to "connect on startup". also, if it is a breaking change, there should be a "breaking" label on the JIRA. finally, i would assume the deprecation to be removed on merge to master for 3.7.0, so perhaps you should consider a follow-on PR to get rid of the warning message and parameter all together on that branch.
separately, there is a merge commit in the history. could you please remove that? other than that VOTE +1
--
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 #1680: TINKERPOP-2708: unhandledRejection upon connection failure - API change - [breaking]
Posted by GitBox <gi...@apache.org>.
xiazcy commented on PR #1680:
URL: https://github.com/apache/tinkerpop/pull/1680#issuecomment-1150431636
> breaking changes are typically documented in the Upgrade Documentation
I have added a section on that for the Upgrade Docs
> finally, i would assume the deprecation to be removed on merge to master for 3.7.0, so perhaps you should consider a follow-on PR to get rid of the warning message and parameter all together on that branch.
I will open a separate PR for master then.
> separately, there is a merge commit in the history. could you please remove that?
Done. Thank you @spmallette !
--
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 a diff in pull request #1680: TINKERPOP-2708: unhandledRejection upon connection failure - API change - [breaking]
Posted by GitBox <gi...@apache.org>.
xiazcy commented on code in PR #1680:
URL: https://github.com/apache/tinkerpop/pull/1680#discussion_r892879796
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js:
##########
@@ -103,13 +103,15 @@ class Connection extends EventEmitter {
this._pingIntervalDelay = this.options.pingInterval || pingIntervalDelay;
this._pongTimeoutDelay = this.options.pongTimeout || pongTimeoutDelay;
- if (this.options.connectOnStartup !== false) {
- this.open();
+ if (this.options.connectOnStartup) {
Review Comment:
Correct, the console warning will not be printed if the user does not set `connectOnStartup`. It will only be displayed if `{connectOnStartup: true}` is set.
--
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 #1680: TINKERPOP-2708: unhandledRejection upon connection failure - API change - [breaking]
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1680:
URL: https://github.com/apache/tinkerpop/pull/1680#issuecomment-1150439765
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1680?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 [#1680](https://codecov.io/gh/apache/tinkerpop/pull/1680?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4fc9e8d) into [3.5-dev](https://codecov.io/gh/apache/tinkerpop/commit/945e588baf7f04e3f95e1f2756b4558bb27df732?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (945e588) will **not change** coverage.
> The diff coverage is `n/a`.
> :exclamation: Current head 4fc9e8d differs from pull request most recent head 199182a. Consider uploading reports for the commit 199182a to get more accurate results
```diff
@@ Coverage Diff @@
## 3.5-dev #1680 +/- ##
========================================
Coverage 63.35% 63.35%
========================================
Files 23 23
Lines 3553 3553
========================================
Hits 2251 2251
Misses 1129 1129
Partials 173 173
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1680?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/1680?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 [945e588...199182a](https://codecov.io/gh/apache/tinkerpop/pull/1680?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