You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by BrynCooke <gi...@git.apache.org> on 2016/10/28 19:08:52 UTC
[GitHub] tinkerpop pull request #470: TINKERPOP-887 ExceptionHandlerStrategy
GitHub user BrynCooke opened a pull request:
https://github.com/apache/tinkerpop/pull/470
TINKERPOP-887 ExceptionHandlerStrategy
Added exception handler strategy to convert FastNoSuchElementExceptions in to regular NoSuchElementExceptions when exiting a traversal.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/BrynCooke/incubator-tinkerpop TINKERPOP-887
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/470.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #470
----
commit 4b80bcd2f12b905a927d61e257c2d4b00f76f609
Author: BrynCooke <br...@gmail.com>
Date: 2016-10-28T19:04:10Z
TINKERPOP-887 Added exception handler strategy to convert FastNoSuchElementExceptions in to regular NoSuchElementExceptions when exiting a traversal.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke commented on the issue:
https://github.com/apache/tinkerpop/pull/470
Retargeting at tp32.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/470
I've done some manual tests and all looks good to me. Although the changes in this PR look scary, it doesn't seem to have a bad performance impact (I couldn't see a performance impact at all).
With a note `CHANGELOG` and in the upgrade docs, this would be a VOTE: +1 from my side.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/470
VOTE +1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/470
LGTM, thus VOTE: +1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke commented on the issue:
https://github.com/apache/tinkerpop/pull/470
Exactly. It's currently a huge pain to figure out where things have gone wrong. Without this patch I have to divide and conquer my code every time I hit a FastNoSuchElementException.
There is no cost to a try/catch block. The construction of the regular NoSuchElementException is expensive, but should only be thrown on the top level traversal.
I actually found no significant difference between branches in terms of performance.
Mine
```
gremlin> graph = TinkerGraph.open()
==>tinkergraph[vertices:0 edges:0]
gremlin> graph.io(gryo()).readGraph('data/grateful-dead.kryo')
==>null
gremlin> h = graph.traversal()
==>graphtraversalsource[tinkergraph[vertices:808 edges:8049], standard]
gremlin> g = graph.traversal().withoutStrategies(LazyBarrierStrategy.class)
==>graphtraversalsource[tinkergraph[vertices:808 edges:8049], standard]
gremlin> clock(100){ h.V().out().out().out().toSet() }
==>3.33155785
gremlin> clock(20){ g.V().out().out().out().toSet() }
==>520.2727724499999
gremlin> clock(20){ g.V().out().flatMap(out()).flatMap(out()).toSet() }
==>903.5254189999999
gremlin> clock(100){ g.V().repeat(out()).times(3).toSet() }
==>1.8392006699999999
gremlin> clock(100){ h.V().count() }
==>0.0069826
gremlin>
```
Master
```
gremlin> graph = TinkerGraph.open()
==>tinkergraph[vertices:0 edges:0]
gremlin> graph.io(gryo()).readGraph('data/grateful-dead.kryo')
==>null
gremlin> h = graph.traversal()
==>graphtraversalsource[tinkergraph[vertices:808 edges:8049], standard]
gremlin> g = graph.traversal().withoutStrategies(LazyBarrierStrategy.class)
==>graphtraversalsource[tinkergraph[vertices:808 edges:8049], standard]
gremlin> clock(100){ h.V().out().out().out().toSet() }
==>3.59260972
gremlin> clock(20){ g.V().out().out().out().toSet() }
==>519.7500263
gremlin> clock(20){ g.V().out().flatMap(out()).flatMap(out()).toSet() }
==>907.32923195
gremlin> clock(100){ g.V().repeat(out()).times(3).toSet() }
==>1.76869779
gremlin> clock(100){ h.V().count() }
==>0.006985369999999999
gremlin>
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/470
I think I already merged before your rebase. Can you simply close this as we have pushed tp32.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/470
Huh, interesting. Sorry, currently not at a point in my day to be able to build and test. Will need some more time to reason through the consequences, but I do appreciate the simplicity and (as @dkuppitz acknowledges), the lack of performance issues.
Question, does this model work with lambda steps? That is:
```
g.V().blah().map{ x -> __.inject(x).will().not().have().anything().next() }
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/470
And for my own education -- this is to show only WHICH traversal "failed," not where in the traversal it failed? Thus, its more about many traversals in an application and knowing which traversal is "bad" ? Correct?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke commented on the issue:
https://github.com/apache/tinkerpop/pull/470
The lambda traversal will throw a regular NoSuchElementException is it is unparented.
However, won't this will only happen once? The exception will bubble up and out of the top level traversal?
If they did this
```
g.V().blah().map{ x ->
try {
__.inject(x).will().not().have().anything().next()
return "foo";
}
catch(NoSuchElementException e) {
return "bar";
}
}
```
instead of
```
g.V().blah().map{ x ->
if(__.inject(x).will().not().have().anything().hasNext()) {
return "foo";
}
else {
return "bar";
}
}
```
Then they would have a problem.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #470: TINKERPOP-887 Conversion of FastNoSuchElementEx...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke closed the pull request at:
https://github.com/apache/tinkerpop/pull/470
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by PommeVerte <gi...@git.apache.org>.
Github user PommeVerte commented on the issue:
https://github.com/apache/tinkerpop/pull/470
looks good and is a nice addition *I will definitely enjoy this* : VOTE +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke commented on the issue:
https://github.com/apache/tinkerpop/pull/470
@okram I have changed the approach entirely to avoid adding an extra strategy.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke commented on the issue:
https://github.com/apache/tinkerpop/pull/470
@dkuppitz I've updated the changelog and upgrade docs. Please could you take a look?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke commented on the issue:
https://github.com/apache/tinkerpop/pull/470
retest this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke commented on the issue:
https://github.com/apache/tinkerpop/pull/470
Great thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 ExceptionHandlerStrategy
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/470
Why is this a default strategy? This should be more of a "turn it on if you are testing." We shouldn't waste clock cycles on every traversal.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/470
Ah, so they want to know WHICH traversal in their application through the exception, not necessarily WHERE in a particular traversal the exception was thrown?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/470
did anyone run a full docker build on this - i didn't see that in the comments? i have one running now....i think we should wait for that before merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/470
All tests pass with `docker/build.sh -t -n -i`
VOTE +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke commented on the issue:
https://github.com/apache/tinkerpop/pull/470
Yes, that's it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/470
I ran a benchmark:
**BRYN'S BRANCH**
```
gremlin> graph = TinkerGraph.open()
==>tinkergraph[vertices:0 edges:0]
gremlin> graph.io(gryo()).readGraph('data/grateful-dead.kryo')
==>null
gremlin> h = graph.traversal()
==>graphtraversalsource[tinkergraph[vertices:808 edges:8049], standard]
gremlin> g = graph.traversal().withoutStrategies(LazyBarrierStrategy.class)
==>graphtraversalsource[tinkergraph[vertices:808 edges:8049], standard]
gremlin>
gremlin> clock(100){ h.V().out().out().out().toSet() }
==>5.142378699999999
gremlin> clock(20){ g.V().out().out().out().toSet() }
==>778.8892011
gremlin> clock(20){ g.V().out().flatMap(out()).flatMap(out()).toSet() }
==>1234.6360252
gremlin> clock(100){ g.V().repeat(out()).times(3).toSet() }
==>2.2864299399999997
gremlin> clock(100){ h.V().count() }
==>0.01190773
```
**MASTER BRANCH**
```
gremlin> graph = TinkerGraph.open()
==>tinkergraph[vertices:0 edges:0]
gremlin> graph.io(gryo()).readGraph('data/grateful-dead.kryo')
==>null
gremlin> h = graph.traversal()
==>graphtraversalsource[tinkergraph[vertices:808 edges:8049], standard]
gremlin> g = graph.traversal().withoutStrategies(LazyBarrierStrategy.class)
==>graphtraversalsource[tinkergraph[vertices:808 edges:8049], standard]
gremlin>
gremlin> clock(100){ h.V().out().out().out().toSet() }
==>3.2528534
gremlin> clock(20){ g.V().out().out().out().toSet() }
==>701.7937965499999
gremlin> clock(20){ g.V().out().flatMap(out()).flatMap(out()).toSet() }
==>1374.86072635
gremlin> clock(100){ g.V().repeat(out()).times(3).toSet() }
==>2.4810170499999997
gremlin> clock(100){ h.V().count() }
==>0.01866702
```
The reason for the `withoutStrategies(LazyBarrierStrategy)` is that I wanted to test lots and lots of results and thus, test the cost of the added `try/catch`-block. I guess the costs are neglible (?? what do others think ??).
Also, @bryncooke, can you provide an example of master/ vs. your branch of when its good to get a `NoSuchElementException`? When I do:
```
gremlin> g.V().out().values('age').next()
java.util.NoSuchElementException
Type ':help' or ':h' for help.
Display stack trace? [yN]y
java.util.NoSuchElementException
at org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal.next(DefaultTraversal.java:194)
...
```
...all I learn is that the traversal `next()` method couldn't find anything. But that is obvious. When is it NOT obvious?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #470: TINKERPOP-887 Conversion of FastNoSuchElementException...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/470
Interesting. I've had different results in myy benchmark and it looked more like just a lot of noise in the measurements, since Bryn's branch was sometimes a bit faster. There's a crazy difference between the 2 results of `h.V().out().out().out().toSet()`, but the others look okay to me.
> When is it NOT obvious?
It is not obvious for the actual users of TinkerPop. Applications throw exceptions and users have no idea where they need to fix their application.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---