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/13 20:52:02 UTC
[GitHub] [tinkerpop] jroimartin opened a new pull request, #1700: gremlin-go: support per-request arguments in bytecode
jroimartin opened a new pull request, #1700:
URL: https://github.com/apache/tinkerpop/pull/1700
The issue that triggered this change is that per-request options were not working with gremlin-go. For instance, setting a per-request timeout this way:
```go
result, err := g.With("evaluationTimeout", 5000).V().Count().Next()
if err != nil {
// Handle error.
}
```
A very similar problem was described some time ago for the Python GLV in the JIRA issue [TINKERPOP-2296](https://issues.apache.org/jira/browse/TINKERPOP-2296) and solved in the pull-request [apache/tinkerpop#1329](https://github.com/apache/tinkerpop/pull/1329).
Stephen Mallette explains the root cause of the issue in a comment in JIRA:
> There is no real strategy application in GLVs - the strategies are simply shipped to the server as bytecode configurations. By that point, it's already bypassed the point where the server will care about the timeout (or other request setting) overrides.
The Python GLV fixes this issue by:
1. Converting the "with" options into an OptionsStrategy.
2. Extracting a whitelisted set of options from the OptionsStrategy.
3. Sending the extracted options as part of the bytecode request.
However, this means that the generated bytecode will not always match the code written by the users. E.g.
```
with(k, v) -> withStrategies(OptionsStrategy({k: v}))
```
So, this PR takes a slightly different approach. It extracts the whitelisted set of per-request options in makeBytecodeRequests() and adds them to the bytecode request without modifying the generated bytecode.
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
xiazcy commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r904160380
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
Thank you!
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#issuecomment-1156773779
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1700?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 [#1700](https://codecov.io/gh/apache/tinkerpop/pull/1700?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (601d6a1) into [3.5-dev](https://codecov.io/gh/apache/tinkerpop/commit/eba3b29b4a3d0aeca55da129c1279351fa7184f0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eba3b29) will **increase** coverage by `0.23%`.
> The diff coverage is `66.66%`.
```diff
@@ Coverage Diff @@
## 3.5-dev #1700 +/- ##
===========================================
+ Coverage 63.35% 63.59% +0.23%
===========================================
Files 23 23
Lines 3553 3601 +48
===========================================
+ Hits 2251 2290 +39
- Misses 1129 1134 +5
- Partials 173 177 +4
```
| [Impacted Files](https://codecov.io/gh/apache/tinkerpop/pull/1700?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage ฮ | |
|---|---|---|
| [gremlin-go/driver/request.go](https://codecov.io/gh/apache/tinkerpop/pull/1700/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-Z3JlbWxpbi1nby9kcml2ZXIvcmVxdWVzdC5nbw==) | `84.76% <66.66%> (-15.24%)` | :arrow_down: |
| [gremlin-go/driver/graphTraversal.go](https://codecov.io/gh/apache/tinkerpop/pull/1700/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-Z3JlbWxpbi1nby9kcml2ZXIvZ3JhcGhUcmF2ZXJzYWwuZ28=) | `89.89% <0.00%> (+0.77%)` | :arrow_up: |
| [gremlin-go/driver/graphTraversalSource.go](https://codecov.io/gh/apache/tinkerpop/pull/1700/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-Z3JlbWxpbi1nby9kcml2ZXIvZ3JhcGhUcmF2ZXJzYWxTb3VyY2UuZ28=) | `85.39% <0.00%> (+4.49%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1700?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/1700?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 [bc4d687...601d6a1](https://codecov.io/gh/apache/tinkerpop/pull/1700?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] jroimartin commented on pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#issuecomment-1158569955
In the meantime I also renamed `extractWithReqArgs` to `extractWithReqArg` given that it returns one single request argument.
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r899121528
##########
gremlin-go/driver/request.go:
##########
@@ -83,6 +88,85 @@ func makeBytecodeRequest(bytecodeGremlin *Bytecode, traversalSource string, sess
}
}
+// allowedReqArgs contains the arguments that will be extracted from the
+// bytecode and sent with the request.
+var allowedReqArgs = map[string]bool{
+ "evaluationTimeout": true,
+ "scriptEvaluationTimeout": true,
Review Comment:
Thanks for the review!
Would it be a problem to keep it for now? If I'm not wrong, this option is still supported by other GLVs and mentioned in the Tinkerpop reference. By supporting it we anticipate to users that try to use the old name and don't understand why the timeout is being ignored or why it does not works with all GLVs. I feel it is better to be consistent among GLVs and, once the setting is fully deprecated, removing the support from all the languages at once. What do you think?
Some instances,
**gremlin-python**
https://github.com/apache/tinkerpop/blob/7ad487b75992a4c46871ae9aab2d5451c133eb8b/gremlin-python/src/main/python/gremlin_python/driver/driver_remote_connection.py#L171
**Tinkerpop reference**
In the different GLV sections:
> The following options are allowed on a per-request basis in this fashion: batchSize, requestId, userAgent and evaluationTimeout (formerly scriptEvaluationTimeout which is also supported but now deprecated).
Which suggests that `scriptEvaluationTimeout` should still work although is not recommended to use it.
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
simonz-bq commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r898466000
##########
gremlin-go/driver/request.go:
##########
@@ -83,6 +88,85 @@ func makeBytecodeRequest(bytecodeGremlin *Bytecode, traversalSource string, sess
}
}
+// allowedReqArgs contains the arguments that will be extracted from the
+// bytecode and sent with the request.
+var allowedReqArgs = map[string]bool{
+ "evaluationTimeout": true,
+ "scriptEvaluationTimeout": true,
+ "batchSize": true,
+ "requestId": true,
+ "userAgent": true,
+}
+
+// extractReqArgs extracts request arguments from the provided bytecode.
+func extractReqArgs(bytecode *Bytecode) map[string]interface{} {
+ args := make(map[string]interface{})
+
+ for _, insn := range bytecode.sourceInstructions {
Review Comment:
Nit: It's good practice to avoid abbreviations.
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r902994138
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
@lyndonbauto sorry, but isn't this essentially the same that I did in 866364ce328c7a91bac62366815224f30c0f9601?
I did it table-driven because this way it is easier to add new test cases (you just have to add a new entry to the table) and everything related to a test case is grouped together (each table entry contains the description of the test case, the traversal and the expected error).
```go
reqArgsTests := []struct {
msg string
traversal *GraphTraversal
nilErr bool
}{
{
"Traversal must time out (With)",
g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
false,
},
{
"Traversal must finish (With)",
g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
true,
},
{
"evaluationTimeout is overridden and traversal must time out (With)",
g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
false,
},
{
"Traversal must time out (OptionsStrategy)",
g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
false,
},
{
"Traversal must finish (OptionsStrategy)",
g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
true,
},
}
```
Of course, if you prefer, I can change it to what you proposed:
```go
err1 := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
err2 := g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
err3 := g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
err4 := g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Itearte()
err5 := g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.NotNil(t, <-err1)
assert.Nil(t, <-err2)
assert.NotNil(t, <-err3)
assert.NotNil(t, <-err4)
assert.Nil(t, <-err5)
```
Which is shorter, but then the asserts are not next to the test cases, which IMHO make it harder to read and maintain. For instance when you want to add a new test case in the middle of the existing ones.
What do you think? Of course, I'm more than happy to change it if that helps to follow the coding guidelines of the project or if I missed something.
Regarding the gremlin-server error. Am I doing anything wrong? Should I send a bug report?
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r902759869
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
I've just checked and I got the same error with gremlin-server at 0a572133aa (3.5-dev branch).
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r903519013
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
I've been comparing the configuration used by the gremlin-go test server and a regular gremlin-server and I think I managed to reproduce the issue with the regular gremlin-server.
In the generated docker container (`tinkerpop/gremlin-server:3.5.4-SNAPSHOT`) I modified `/opt/gremlin-server/conf/gremlin-server.yaml` in the following way:
```diff
--- gremlin-server.yaml 2022-06-22 11:30:53.452370098 +0200
+++ gremlin-server-repro.yaml 2022-06-22 11:27:50.000000000 +0200
@@ -18,7 +18,7 @@
host: 172.17.0.2
port: 8182
evaluationTimeout: 30000
-channelizer: org.apache.tinkerpop.gremlin.server.channel.WebSocketChannelizer
+channelizer: org.apache.tinkerpop.gremlin.server.channel.UnifiedChannelizer
graphs: {
graph: conf/tinkergraph-empty.properties}
scriptEngines: {
```
Again I'm not familiar with the code base of the project, but I hope this can serve as a hint for you.
BTW given that this looks like a race condition and it is not always easy to reproduce them in a reliable way, take this information with a grain of salt. I don't want to waste people time with inaccurate information :slightly_smiling_face:
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r902069270
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
Does this test take 5000 ms * 5 seconds to execute? If so can we possible use `Iterate()` and then check the error promise to run the requests async?
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r903362909
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
@xiazcy very good point. Actually, I have **not** been able to reproduce it with a bare gremlin-server. Specifically, I used:
```
$ docker run --rm -ti -p 8182:8182 tinkerpop/gremlin-server:3.5.4-SNAPSHOT
```
and
```
$ docker run --rm -ti -p 8182:8182 tinkerpop/gremlin-server:3.5.3
```
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
simonz-bq commented on PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#issuecomment-1156985391
Overall, from a code perspective LGTM. Will let someone more familiar with the feature itself review as well.
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r902485983
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
I pushed 866364c. Is that what you mean?
However, now the test is flaky due to what I think is an unrelated issue. gremlin-server fails sometimes with:
```
gremlin-test-server | [ERROR] GremlinGroovyScriptEngine$GroovyCacheLoader - Script compilation FAILED gremlinscriptengine__ggremlinscriptengine__g.with.with("evaluationTimeout","evaluationTimeout",10000L10L))..injectinject((1L1L)).sideEffect(.sideEffect({Thread.sleep(5000)}).none(){Thread.sleep(5000)}).none() took 3ms {}
gremlin-test-server | org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
gremlin-test-server | Script4.groovy: 1: expecting ')', found '10L' @ line 1, column 102.
gremlin-test-server | ut","evaluationTimeout",10000L10L))..inj
gremlin-test-server | ^
gremlin-test-server |
gremlin-test-server | 1 error
gremlin-test-server |
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.failIfErrors(ErrorCollector.java:311)
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.addFatalError(ErrorCollector.java:151)
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:121)
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:133)
...
```
It seems to be trying to compile the following traversal: `gremlinscriptengine__ggremlinscriptengine__g.with.with("evaluationTimeout","evaluationTimeout",10000L10L))..injectinject((1L1L)).sideEffect(.sideEffect({Thread.sleep(5000)}).none(){Thread.sleep(5000)}).none()`, which is obviously wrong. I don't know enough about the code base, but it looks like a data race while building the query string.
I ran the tests against gremlin-server 3.5.3.
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r902485983
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
I pushed 866364c. Is that what you mean?
However, now the test is flaky due to what I think is an unrelated issue. The gremlin-server fails sometimes with:
```
gremlin-test-server | [ERROR] GremlinGroovyScriptEngine$GroovyCacheLoader - Script compilation FAILED gremlinscriptengine__ggremlinscriptengine__g.with.with("evaluationTimeout","evaluationTimeout",10000L10L))..injectinject((1L1L)).sideEffect(.sideEffect({Thread.sleep(5000)}).none(){Thread.sleep(5000)}).none() took 3ms {}
gremlin-test-server | org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
gremlin-test-server | Script4.groovy: 1: expecting ')', found '10L' @ line 1, column 102.
gremlin-test-server | ut","evaluationTimeout",10000L10L))..inj
gremlin-test-server | ^
gremlin-test-server |
gremlin-test-server | 1 error
gremlin-test-server |
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.failIfErrors(ErrorCollector.java:311)
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.addFatalError(ErrorCollector.java:151)
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:121)
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:133)
...
```
It seems to be trying to compile the following traversal: `gremlinscriptengine__ggremlinscriptengine__g.with.with("evaluationTimeout","evaluationTimeout",10000L10L))..injectinject((1L1L)).sideEffect(.sideEffect({Thread.sleep(5000)}).none(){Thread.sleep(5000)}).none()`, which is obviously wrong. I don't know enough about the code base, but it looks like a data race while building the query string.
I ran the tests against gremlin-server 3.5.3.
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r903362909
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
@xiazcy very good point. Actually, I have **not** been able to reproduce it with a bare gremlin-server. Specifically, I tried:
```
$ docker run --rm -ti -p 8182:8182 tinkerpop/gremlin-server:3.5.4-SNAPSHOT
```
and
```
$ docker run --rm -ti -p 8182:8182 tinkerpop/gremlin-server:3.5.3
```
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r903557911
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
Full backtrace of one of the errors:
```
org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
Script4.groovy: 1: unexpected token: . @ line 1, column 53.
.inject((int) 1).sideE.inject(.inject((i
^
1 error
at org.codehaus.groovy.control.ErrorCollector.failIfErrors(ErrorCollector.java:311)
at org.codehaus.groovy.control.ErrorCollector.addFatalError(ErrorCollector.java:151)
at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:121)
at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:133)
at org.codehaus.groovy.control.SourceUnit.addError(SourceUnit.java:325)
at org.codehaus.groovy.antlr.AntlrParserPlugin.transformCSTIntoAST(AntlrParserPlugin.java:224)
at org.codehaus.groovy.antlr.AntlrParserPlugin.parseCST(AntlrParserPlugin.java:192)
at org.codehaus.groovy.control.SourceUnit.parse(SourceUnit.java:226)
at org.codehaus.groovy.control.CompilationUnit$1.call(CompilationUnit.java:201)
at org.codehaus.groovy.control.CompilationUnit.applyToSourceUnits(CompilationUnit.java:965)
at org.codehaus.groovy.control.CompilationUnit.doPhaseOperation(CompilationUnit.java:642)
at org.codehaus.groovy.control.CompilationUnit.processPhaseOperations(CompilationUnit.java:618)
at org.codehaus.groovy.control.CompilationUnit.compile(CompilationUnit.java:595)
at groovy.lang.GroovyClassLoader.doParseClass(GroovyClassLoader.java:401)
at groovy.lang.GroovyClassLoader.access$300(GroovyClassLoader.java:89)
at groovy.lang.GroovyClassLoader$5.provide(GroovyClassLoader.java:341)
at groovy.lang.GroovyClassLoader$5.provide(GroovyClassLoader.java:338)
at org.codehaus.groovy.runtime.memoize.ConcurrentCommonCache.getAndPut(ConcurrentCommonCache.java:147)
at groovy.lang.GroovyClassLoader.parseClass(GroovyClassLoader.java:336)
at groovy.lang.GroovyClassLoader.parseClass(GroovyClassLoader.java:320)
at groovy.lang.GroovyClassLoader.parseClass(GroovyClassLoader.java:262)
at org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine$GroovyCacheLoader.lambda$load$0(GremlinGroovyScriptEngine.java:821)
at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1700)
at java.base/java.util.concurrent.CompletableFuture.asyncSupplyStage(CompletableFuture.java:1714)
at java.base/java.util.concurrent.CompletableFuture.supplyAsync(CompletableFuture.java:1931)
at org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine$GroovyCacheLoader.load(GremlinGroovyScriptEngine.java:819)
at org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine$GroovyCacheLoader.load(GremlinGroovyScriptEngine.java:814)
at com.github.benmanes.caffeine.cache.BoundedLocalCache$BoundedLocalLoadingCache.lambda$new$0(BoundedLocalCache.java:3117)
at com.github.benmanes.caffeine.cache.LocalCache.lambda$statsAware$0(LocalCache.java:144)
at com.github.benmanes.caffeine.cache.BoundedLocalCache.lambda$doComputeIfAbsent$16(BoundedLocalCache.java:1968)
at java.base/java.util.concurrent.ConcurrentHashMap.compute(ConcurrentHashMap.java:1908)
at com.github.benmanes.caffeine.cache.BoundedLocalCache.doComputeIfAbsent(BoundedLocalCache.java:1966)
at com.github.benmanes.caffeine.cache.BoundedLocalCache.computeIfAbsent(BoundedLocalCache.java:1949)
at com.github.benmanes.caffeine.cache.LocalCache.computeIfAbsent(LocalCache.java:113)
at com.github.benmanes.caffeine.cache.LocalLoadingCache.get(LocalLoadingCache.java:67)
at org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine.getScriptClass(GremlinGroovyScriptEngine.java:569)
at org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine.eval(GremlinGroovyScriptEngine.java:376)
at java.scripting/javax.script.AbstractScriptEngine.eval(AbstractScriptEngine.java:233)
at org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine.eval(GremlinGroovyScriptEngine.java:298)
at org.apache.tinkerpop.gremlin.server.handler.AbstractSession.fromBytecode(AbstractSession.java:443)
at org.apache.tinkerpop.gremlin.server.handler.AbstractSession.process(AbstractSession.java:264)
at org.apache.tinkerpop.gremlin.server.handler.SingleTaskSession.run(SingleTaskSession.java:70)
at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at java.base/java.lang.Thread.run(Thread.java:829)
```
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r903972598
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
> The UnifiedChannelizer is need because we need the HTTP endpoint to perform the container wait-for check in docker compose, to make sure server with neo4j transactions is up before running the integration tests.
@xiazcy what about using `WsAndHttpChannelizer`? I could not reproduce the bug with it.
I mean meanwhile the `UnifiedChannelizer` issue is investigated.
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r902994138
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
@lyndonbauto sorry, but isn't this essentially the same that I did in 866364ce328c7a91bac62366815224f30c0f9601?
I did it table-driven because this way it is easier to add new test cases (you just have to add a new entry to the table) and everything related to a test case is grouped together (each table entry contains the description of the test case, the traversal and the expected error).
```go
reqArgsTests := []struct {
msg string
traversal *GraphTraversal
nilErr bool
}{
{
"Traversal must time out (With)",
g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
false,
},
{
"Traversal must finish (With)",
g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
true,
},
{
"evaluationTimeout is overridden and traversal must time out (With)",
g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
false,
},
{
"Traversal must time out (OptionsStrategy)",
g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
false,
},
{
"Traversal must finish (OptionsStrategy)",
g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
true,
},
}
```
Of course, if you prefer, I can change it to what you proposed:
```go
err1 := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
err2 := g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
err3 := g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
err4 := g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Itearte()
err5 := g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.NotNil(t, <-err1)
assert.Nil(t, <-err2)
assert.NotNil(t, <-err3)
assert.NotNil(t, <-err4)
assert.Nil(t, <-err5)
```
Which is shorter, but then the asserts are not next to the test cases, which IMHO make it harder to read and maintain. For instance when you want to add a new test case in the middle of the existing ones or the number of test cases gets bigger.
What do you think? Of course, I'm more than happy to change it if that helps to follow the coding guidelines of the project or if I missed something.
Regarding the gremlin-server error. Am I doing anything wrong? Should I send a bug report?
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r903005267
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
Oh sorry I missed the iterate calls, my bad! I am not sure about the gremlin-server error tbh. @spmallette have you seen anything like this before? odd that it is inconsistent. I wonder if we can reproduce it with another glv.
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r902916377
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
@jroimartin no that wasn't what I meant, I should have been more specific.
Take a look at the [Iterate() function](https://pkg.go.dev/github.com/apache/tinkerpop/gremlin-go/v3/driver#Traversal.Iterate). This is typically used when someone is inserting data, it ignores the return value and runs in the background, providing a channel with an error instead of blocking and returning an error.
It is use **IN PLACE** of `Next()`
How this should look in code (untested, so might need minor edit to compile):
```
, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
assert.NotNil(t, err)
_, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
assert.Nil(t, err)
```
could instead be something like:
```
err1 := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
err2 := g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
....
assert.Nil(t, <-err1)
assert.Nil(t, <-err2)
....
```
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
xiazcy commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r903919770
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
Thanks @jroimartin! This is interesting. The UnifiedChannelizer is need because we need the HTTP endpoint to perform the container wait-for check in docker compose, to make sure server with neo4j transactions is up before running the integration tests. So we may need to stick with the current configuration, although this is probably an issue since it doesn't error on the default web-socket.
Stephen @spmallette would you have any quick thoughts regarding this floating race condition in UnifiedChannelizer? We could dig into this more and open a JIRA ticket for it.
In the meanwhile, I think we should get this PR moving forward. And unfortunately that may mean we submit the requests synchronously and wait on the err channel of each one, instead of in parallel.
`
err1 := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.NotNil(t, <-err1)
err2 := g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.Nil(t, <-err2)
err3 := g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.NotNil(t, <-err3)
err4 := g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.NotNil(t, <-err4)
err5 := g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.Nil(t, <-err5)
`
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r902485983
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
I pushed 866364c. Is that what you mean?
However, now the test is flaky due to what I think is an unrelated issue. gremlin-server fails sometimes with:
```
gremlin-test-server | [ERROR] GremlinGroovyScriptEngine$GroovyCacheLoader - Script compilation FAILED gremlinscriptengine__ggremlinscriptengine__g.with.with("evaluationTimeout","evaluationTimeout",10000L10L))..injectinject((1L1L)).sideEffect(.sideEffect({Thread.sleep(5000)}).none(){Thread.sleep(5000)}).none() took 3ms {}
gremlin-test-server | org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
gremlin-test-server | Script4.groovy: 1: expecting ')', found '10L' @ line 1, column 102.
gremlin-test-server | ut","evaluationTimeout",10000L10L))..inj
gremlin-test-server | ^
gremlin-test-server |
gremlin-test-server | 1 error
gremlin-test-server |
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.failIfErrors(ErrorCollector.java:311)
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.addFatalError(ErrorCollector.java:151)
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:121)
gremlin-test-server | at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:133)
...
```
In this case it seems to be trying to compile the following traversal: `gremlinscriptengine__ggremlinscriptengine__g.with.with("evaluationTimeout","evaluationTimeout",10000L10L))..injectinject((1L1L)).sideEffect(.sideEffect({Thread.sleep(5000)}).none(){Thread.sleep(5000)}).none()`, which is obviously wrong. I don't know enough about the code base, but it looks like a data race while building the query string.
I ran the tests against gremlin-server 3.5.3.
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r902994138
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
@lyndonbauto sorry, but isn't this essentially the same that I did in 866364ce328c7a91bac62366815224f30c0f9601?
I did it table-driven because this way it is easier to add new test cases (you just have to add a new entry to the table) and everything related to a test case is grouped together (each table entry contains the description of the test case, the traversal and the expected error).
```go
reqArgsTests := []struct {
msg string
traversal *GraphTraversal
nilErr bool
}{
{
"Traversal must time out (With)",
g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
false,
},
{
"Traversal must finish (With)",
g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
true,
},
{
"evaluationTimeout is overridden and traversal must time out (With)",
g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
false,
},
{
"Traversal must time out (OptionsStrategy)",
g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
false,
},
{
"Traversal must finish (OptionsStrategy)",
g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}),
true,
},
}
```
Of course, if you prefer, I can change it to what you proposed:
```go
err1 := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
err2 := g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
err3 := g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
err4 := g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Itearte()
err5 := g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.NotNil(t, <-err1)
assert.Nil(t, <-err2)
assert.NotNil(t, <-err3)
assert.NotNil(t, <-err4)
assert.Nil(t, <-err5)
```
Which is shorter, but then the asserts are not next to the test cases, which IMHO make it harder to read and maintain. For instance when you want to add a new test case in the middle of the existing ones.
What do you think? Of course, I'm more than happy to change it if that helps to follow the coding guidelines of the project of if I missed something.
Regarding the gremlin-server error. Am I doing anything wrong? Should I send a bug report?
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
xiazcy commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r903184326
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
Hi @jroimartin, just to make sure it's not due to the gremlin-go test server set up. Have you tried running the python script against the built gremlin server it self, instead of through docker compose? This would be running the file `gremlin-server/target/apache-tinkerpop-gremlin-server-3.5.4-SNAPSHOT-standalone/bin/gremlin-server.sh`, just starting the default server should be enough for the inject. Note the port will be different, `8182` instead of `45940`.
--
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 merged pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
spmallette merged PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700
--
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] jroimartin commented on pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#issuecomment-1157744750
> Normally I'd suggest a CHANGELOG entry or even upgrade docs, but since the Go GLV isn't officially released yet, we can forego that as you've basically just added a feature that should have been present anyway from the start.
I've just checked the docs and it is already there! :smile:
https://github.com/apache/tinkerpop/blob/24384fb9932e028c753ae73c0fa7817665e80713/docs/src/reference/gremlin-variants.asciidoc?plain=1#L1457-L1466
--
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 a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r899215277
##########
gremlin-go/driver/request.go:
##########
@@ -83,6 +88,85 @@ func makeBytecodeRequest(bytecodeGremlin *Bytecode, traversalSource string, sess
}
}
+// allowedReqArgs contains the arguments that will be extracted from the
+// bytecode and sent with the request.
+var allowedReqArgs = map[string]bool{
+ "evaluationTimeout": true,
+ "scriptEvaluationTimeout": true,
Review Comment:
`scriptEvaluationTimeout` was technically removed in 3.5.0: https://issues.apache.org/jira/browse/TINKERPOP-2295 if there are remnants of it still around, that is probably a mistake. there are some graphs that quietly still support it in 3.5.x, but i don't think TinkerPop needs to continue to quietly do the same. and gremlin-go is new so why bother to allow folks to write code that uses deprecated/removed things. Just seems like we'd be adding things to forget to remove later ๐
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r903972598
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
> The UnifiedChannelizer is need because we need the HTTP endpoint to perform the container wait-for check in docker compose, to make sure server with neo4j transactions is up before running the integration tests.
@xiazcy what about using `WsAndHttpChannelizer`? I could not reproduce the bug with it.
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r903067178
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
@lyndonbauto I've been able to reproduce it using gremlin-python with the following set-up (it's a quick hack):
**repro.py**
```python
#!/usr/bin/env python3
from gremlin_python.driver.driver_remote_connection import DriverRemoteConnection
from gremlin_python.process.anonymous_traversal import traversal
g = traversal().with_remote(DriverRemoteConnection('ws://localhost:45940/gremlin','g'))
try:
g.inject(1).sideEffect(lambda: "Thread.sleep(1000)").iterate()
except Exception as e:
print(e)
```
**stress.bash**
```bash
#!/bin/bash
for i in $(seq 0 250); do
python3 repro.py &
pids[${i}]=$!
done
wait ${pids[*]}
```
I'm running gremlin-server this way (from the gremlin-go directory):
```
$ GREMLIN_SERVER=3.5.4-SNAPSHOT docker-compose run --rm --service-ports gremlin-test-server
```
The reproducer:
```
$ ./stress.bash
Received error message '{'requestId': '0a358421-df3c-4965-8535-bf66337289c8', 'status': {'message': 'startup failed:\nScript4.groovy: 1: unexpected token: 1 @ line 1, column 44.\n1 error\n', 'code': 597, 'attributes': {'stackTrace': 'org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:\nScript4.groovy: 1: unexpected token: 1 @ line 1, column 44.\n engine__g.inject((int) 1(int) 1).sideEff\n ^\n\n1 error\n\n\tat org.codehaus.groovy.control.ErrorCollector.failIfErrors(ErrorCollector.java:311)\n\tat org.codehaus.groovy.control.ErrorCollector.addFatalError(ErrorCollector.java:151)\n\tat org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:121)\n\tat org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:133)\n\tat org.codehaus.groovy.control.So
...
```
In the gremlin-server logs:
```
[ERROR] ? - Script compilation FAILED gremlinscriptengine__g.inject((int) 1(int) 1).sideEffect().sideEffect({Thread.sleep(1000)}{Thread.sleep(1000)}))..nonenone()() took 11ms {}
org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
Script4.groovy: 1: unexpected token: 1 @ line 1, column 44.
engine__g.inject((int) 1(int) 1).sideEff
^
1 error
at org.codehaus.groovy.control.ErrorCollector.failIfErrors(ErrorCollector.java:311)
at org.codehaus.groovy.control.ErrorCollector.addFatalError(ErrorCollector.java:151)
at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:121)
at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:133)
at org.codehaus.groovy.control.SourceUnit.addError(SourceUnit.java:325)
...
```
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
spmallette commented on PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#issuecomment-1157585246
> It extracts the whitelisted set of per-request options in makeBytecodeRequests() and adds them to the bytecode request without modifying the generated bytecode.
I don't think that's a bad approach and I can't think of any problems with this offhand. Perhaps python (other GLVs) should be done that way too. Generally speaking, it's nicer when the bytecode matches what was originally constructed.
Normally I'd suggest a CHANGELOG entry or even upgrade docs, but since the Go GLV isn't officially released yet, we can forego that as you've basically just added a feature that should have been present anyway from the start. ๐
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] spmallette commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r899007828
##########
gremlin-go/driver/request.go:
##########
@@ -83,6 +88,85 @@ func makeBytecodeRequest(bytecodeGremlin *Bytecode, traversalSource string, sess
}
}
+// allowedReqArgs contains the arguments that will be extracted from the
+// bytecode and sent with the request.
+var allowedReqArgs = map[string]bool{
+ "evaluationTimeout": true,
+ "scriptEvaluationTimeout": true,
Review Comment:
i wouldn't bother supporting `scriptEvaluationTimeout` - it's a deprecated argument where `evaluationTimeout` is preferred.
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r899121528
##########
gremlin-go/driver/request.go:
##########
@@ -83,6 +88,85 @@ func makeBytecodeRequest(bytecodeGremlin *Bytecode, traversalSource string, sess
}
}
+// allowedReqArgs contains the arguments that will be extracted from the
+// bytecode and sent with the request.
+var allowedReqArgs = map[string]bool{
+ "evaluationTimeout": true,
+ "scriptEvaluationTimeout": true,
Review Comment:
Thanks for the review!
Would it be a problem to keep it for now? If I'm not wrong, this option is still supported by other GLVs and mentioned in the Tinkerpop reference. By supporting it we anticipate to users that try to use the old name and don't understand why the timeout is being ignored or why it does not work with all GLVs. I feel it is better to be consistent among GLVs and, once the setting is fully deprecated, removing the support from all the languages at once. What do you think?
Some instances,
**gremlin-python**
https://github.com/apache/tinkerpop/blob/7ad487b75992a4c46871ae9aab2d5451c133eb8b/gremlin-python/src/main/python/gremlin_python/driver/driver_remote_connection.py#L171
**Tinkerpop reference**
In the different GLV sections:
> The following options are allowed on a per-request basis in this fashion: batchSize, requestId, userAgent and evaluationTimeout (formerly scriptEvaluationTimeout which is also supported but now deprecated).
Which suggests that `scriptEvaluationTimeout` should still work although is not recommended to use it.
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r899332567
##########
gremlin-go/driver/request.go:
##########
@@ -83,6 +88,85 @@ func makeBytecodeRequest(bytecodeGremlin *Bytecode, traversalSource string, sess
}
}
+// allowedReqArgs contains the arguments that will be extracted from the
+// bytecode and sent with the request.
+var allowedReqArgs = map[string]bool{
+ "evaluationTimeout": true,
+ "scriptEvaluationTimeout": true,
Review Comment:
That makes sense. I removed `scriptEvaluationTimeout` both from the gremlin-go docs and code. I didn't touch any other GLV because it feels like it should go in a different PR.
eb9b358 should fix it. PTAL :slightly_smiling_face:
Thanks!
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
xiazcy commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r904003590
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
@jroimartin Actually yes, `WsAndHttpChannelizer` would work, and since ultimately we just need HTTP requests enabled, we may not even need to switch back to `UnifiedChannelizer`. Could you make that change as well with your PR? Thank you!
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
xiazcy commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r903919770
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
Thanks @jroimartin! This is interesting. The UnifiedChannelizer is need because we need the HTTP endpoint to perform the container wait-for check in docker compose, to make sure server with neo4j transactions is up before running the integration tests. So we may need to stick with the current configuration, although this is probably an issue since it doesn't error on the default web-socket.
Stephen @spmallette would you have any quick thoughts regarding this floating race condition in UnifiedChannelizer? We could dig into this more and open a JIRA ticket for it.
In the meanwhile, I think we should get this PR moving forward. And unfortunately that may mean we submit the requests synchronously and wait on the err channel of each one, instead of in parallel.
```
err1 := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.NotNil(t, <-err1)
err2 := g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.Nil(t, <-err2)
err3 := g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.NotNil(t, <-err3)
err4 := g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.NotNil(t, <-err4)
err5 := g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Iterate()
assert.Nil(t, <-err5)
```
--
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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r904033476
##########
gremlin-go/driver/connection_test.go:
##########
@@ -1121,4 +1121,26 @@ func TestConnection(t *testing.T) {
// This routine.
assert.Equal(t, startCount, runtime.NumGoroutine())
})
+
+ t.Run("Test per-request arguments", func(t *testing.T) {
+ skipTestsIfNotEnabled(t, integrationTestSuiteName, testNoAuthEnable)
+
+ g := getTestGraph(t, testNoAuthUrl, testNoAuthAuthInfo, testNoAuthTlsConfig)
+ defer g.remoteConnection.Close()
+
+ _, err := g.With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+
+ _, err = g.With("evaluationTimeout", 10000).With("evaluationTimeout", 10).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.NotNil(t, err)
+
+ _, err = g.WithStrategies(OptionsStrategy(map[string]interface{}{"evaluationTimeout": 10000})).Inject(1).SideEffect(&Lambda{"Thread.sleep(5000)", "gremlin-groovy"}).Next()
+ assert.Nil(t, err)
+ })
Review Comment:
Done in 30f9702. PTAL.
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#issuecomment-1163653347
> LGTM. @lyndonbauto would you have any additional comments? Thanks! Also I've opened a ticket to track the UnifiedChannelizer issue: https://issues.apache.org/jira/browse/TINKERPOP-2765. It will be looked at separately since moving forward it shouldn't affect gremlin-go with our changed to `WsAndHttpChannelizer`.
I am good.
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
xiazcy commented on PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#issuecomment-1163527493
LGTM.
@lyndonbauto would you have any additional comments? Thanks!
Also I've opened a ticket to track the UnifiedChannelizer issue: https://issues.apache.org/jira/browse/TINKERPOP-2765. It will be looked at separately since moving forward it shouldn't affect gremlin-go with our changed to `WsAndHttpChannelizer`.
--
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 #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
xiazcy commented on PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#issuecomment-1156785816
Thank you so much for submitting the PR @jroimartin, we have enabled the GitHub workflows and will be taking a closer look into 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] jroimartin commented on a diff in pull request #1700: gremlin-go: support per-request arguments in bytecode
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1700:
URL: https://github.com/apache/tinkerpop/pull/1700#discussion_r898489406
##########
gremlin-go/driver/request.go:
##########
@@ -83,6 +88,85 @@ func makeBytecodeRequest(bytecodeGremlin *Bytecode, traversalSource string, sess
}
}
+// allowedReqArgs contains the arguments that will be extracted from the
+// bytecode and sent with the request.
+var allowedReqArgs = map[string]bool{
+ "evaluationTimeout": true,
+ "scriptEvaluationTimeout": true,
+ "batchSize": true,
+ "requestId": true,
+ "userAgent": true,
+}
+
+// extractReqArgs extracts request arguments from the provided bytecode.
+func extractReqArgs(bytecode *Bytecode) map[string]interface{} {
+ args := make(map[string]interface{})
+
+ for _, insn := range bytecode.sourceInstructions {
Review Comment:
Thanks for the review!
Do you mean using `insn` instead of `instruction` or `sourceInstruction`?
I usually try to stick to the following criteria described by Russ Cox:
> Descriptive names are fine for things that merit description at every mention.
>
> Receiver names typically donโt, nor do names scoped to a small amount of code (like a small loop body or even a small function).
(Reference: https://twitter.com/_rsc/status/1507319374612414472)
Also, I didn't want to shadow the `instruction` type.
That being said, I'm more than happy to change it if that fits better with the coding style of the project. What do you suggest?
--
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