You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by spmallette <gi...@git.apache.org> on 2016/04/30 12:37:16 UTC
[GitHub] incubator-tinkerpop pull request: TINKERPOP-946 Interrupting Trave...
GitHub user spmallette opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/299
TINKERPOP-946 Interrupting Traversals
https://issues.apache.org/jira/browse/TINKERPOP-946
This is a mostly complete implementation of traversal interruption. I think it will cover most use cases and as such makes it a candidate for merging back to master. I think the areas where it is lacking is around cancellation of jobs in giraph/hadoop and perhaps some corner cases in existing steps which might have been missed somehow. I think we can just open separate jira tickets for these things as they arise. I have at least one or two in mind already.
I came up with methods to test interruption with unit tests (and it's easy to add new ones) but an easy way to test this out in the console is to do something like:
```text
gremlin> t = new Thread({
gremlin> try {
gremlin> println g.V().out().out().toList()
gremlin> } catch (Exception ex) { ex.printStackTrace() }})
==>Thread[Thread-91,5,main]
gremlin> t.start()
==>null
gremlin> t.interrupt()
==>null
gremlin> org.apache.tinkerpop.gremlin.process.traversal.util.TraversalInterruptedException
at org.apache.tinkerpop.gremlin.process.computer.traversal.step.map.VertexProgramStep.processNextStart(VertexProgramStep.java:80)
at org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep.hasNext(AbstractStep.java:143)
at org.apache.tinkerpop.gremlin.process.traversal.step.util.ExpandableStepIterator.next(ExpandableStepIterator.java:50)
at org.apache.tinkerpop.gremlin.process.computer.traversal.step.map.ComputerResultStep.processNextStart(ComputerResultStep.java:70)
at org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep.next(AbstractStep.java:128)
at org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep.next(AbstractStep.java:38)
at org.apache.tinkerpop.gremlin.process.traversal.Traversal.fill(Traversal.java:146)
at org.apache.tinkerpop.gremlin.process.traversal.Traversal.toList(Traversal.java:103)
at org.apache.tinkerpop.gremlin.process.traversal.Traversal$toList.call(Unknown Source)
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:117)
at groovysh_evaluate$_run_closure1.doCall(groovysh_evaluate:5)
at groovysh_evaluate$_run_closure1.doCall(groovysh_evaluate)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:93)
at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:325)
at org.codehaus.groovy.runtime.metaclass.ClosureMetaClass.invokeMethod(ClosureMetaClass.java:294)
at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1021)
at groovy.lang.Closure.call(Closure.java:426)
at groovy.lang.Closure.call(Closure.java:420)
at groovy.lang.Closure.run(Closure.java:507)
at java.lang.Thread.run(Thread.java:745)
```
The above is a test of interrupting `SparkGraphComputer` - works well and kills the remotely executing spark job.
`RemoteGraph` interruption may need some specific attention. There is no way to remote kill a job so the best that will happen is the client thread gets released.
Ran full integration tests to success: `mvn clean install -DskipIntegrationTests=false`
VOTE +1
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-946
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/299.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 #299
----
commit cc533c7574f4c32d367a4d1ac77d3e70daef3e8b
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-04-18T12:50:33Z
Added support for Traversal interruption.
Check for Thread.interrupted() in loops within steps and throw an unchecked TraversalInterruptedException. Would be better to throw InterruptedException but that would involve a breaking change which would be nice to avoid.
commit fd16fabd7595470bd33f30b882b1f0297d05b55a
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-04-19T23:09:20Z
Added process tests for traversal interruption
Covered GraphStep, VertexStep and PropertiesStep which should yield some coverage to provider implementations.
commit 7a9f7de6ebed5d4293708524c772db0f0b2c3bac
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-04-20T11:52:01Z
Refactored Traversal interruption tests.
Provided better coverage and easier maintenance by making the tests parameterized.
commit db6dfbcea68d1d698cc9eabcfd47ddaf6e4476e6
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-04-22T19:26:35Z
OLAP Traversals now support traversal interruption.
Implemented for SparkGraphComputer and TinkerGraphComputer.
commit e235c128ee148399a385df7e4f546926bda8a6fe
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-04-22T19:49:29Z
Cleaned up OptOuts around traversal interruption test for giraph/spark.
commit fd30fc289eac25baba34a5cb88f6deff80b571c7
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-04-30T10:31:27Z
Added a couple more OptOuts.
The interruption tests are not good for RemoteGraph or HadoopGraph. The interruption models are different there given their different processing. Those might need specific testing.
----
---
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] incubator-tinkerpop pull request: TINKERPOP-946 Interrupting Trave...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/299#issuecomment-216502810
I just pushed a change as you suggested @okram
---
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] incubator-tinkerpop pull request: TINKERPOP-946 Interrupting Trave...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/299#issuecomment-215975367
`docker/build.sh -t -i -n` succeeded.
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] incubator-tinkerpop pull request: TINKERPOP-946 Interrupting Trave...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/299#issuecomment-216893595
Ok - changes are good with: `$ docker/build.sh -t -i -n`
---
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] incubator-tinkerpop pull request: TINKERPOP-946 Interrupting Trave...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/299
---
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] incubator-tinkerpop pull request: TINKERPOP-946 Interrupting Trave...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/299#issuecomment-215977029
I don't think you need to put as may interrupt checks. Most steps inherit from `AbstractStep` and thus, if `AbstractStep.next()` and `AbstractStep.hasNext()` have the interrupt check, then all the `processNextStart()` in subclasses don't need to check for the interrupt. This should greatly reduce the amount of classes touched and requirements on providers with their own step implementations. In short, they just need to make sure they extend `AbstractStep`. However, with that said, I still think you need the `VertexProgramStep` work in there as you have 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] incubator-tinkerpop pull request: TINKERPOP-946 Interrupting Trave...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/299#issuecomment-216542972
You don't need it in `ComputerResultStep` either as that is a simple `AbstractStep` and thus, gets the "interrupt check" from `next()/hasNext()` of `AbstractStep`.
VOTE +1. -- very cool we finally have this. Also, super glad its not some insane reengineering of all the steps.
---
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] incubator-tinkerpop pull request: TINKERPOP-946 Interrupting Trave...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/299#issuecomment-216825501
Removed the `ComputerResultStep` interruption check and rebased. Waiting for a clean build then will 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.
---