Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-971

TraversalSource should be fluent like GraphComputer

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.1.0-incubating
    • Fix Version/s: 3.2.0-incubating
    • Component/s: process
    • Labels:

      Description

      I just realized something so obvious. TraversalSource should be fluent and not this awkward TraversalSource.Builder model we use. You should be able to do this:

      graph = GraphFactory.open(...)
      g = graph.traversal()
      g = g.withStrategy(MyStrategy.class)
      g = g.withSack(1.0,sum)
      ...
      g.V().out().sack()
      g.V().out().out().drop()
      

      Thus, TraversalSource methods return a TraversalSource.

      g = graph.traversal(computer(GiraphGraphComputer)).withStrategy(MyStrategy.class).withSack(1.0,sum).withBulk(false)
      

      That g is then "locked" with those parameterizations and any V()/addV()/etc. off of it will spawn traversal with that parameterization.

      This solves:
      TINKERPOP3-862
      TINKERPOP3-960 (makes more elegant)

      This would be backwards compatible. Though, deprecation would occur.

      Finally, DSLs are still respected.

      g = graph.traversal(SocialTraversal.class)
      

      A fleeting thought...

      g = graph.traversal().using(GiraphGraphComputer)
      g = graph.traversal().via(GremlinServerConnection).using(GiraphGraphComputer)
      

      So much cleaner than all that Builder-crap....

        Issue Links

          Activity

          Hide
          okram Marko A. Rodriguez added a comment - - edited

          This is how things should be done.

          g = graph.traversal().
                             compute(graph -> graph.compute(SparkGraphComputer.class).
                                                             workers(10).
                                                             configure("gremlin.spark.persistContext",true)).
                             bulk(BigIntegerBulk.class).
                             withStrategies(LazyBarrierStrategy.instance(), EventStrategy.instance()).
                             withoutStrategies(MatchPredicateStrategy.class, AdjacentIncidentStrategy.class).
                             sack(ArrayList::new, ArrayList::clone, (a,b) -> a.addAll(b))
          

          ....for DSLs, it would be the same save:

          g = graph.traversal(SocialTraversal.class).....
          
          Show
          okram Marko A. Rodriguez added a comment - - edited This is how things should be done. g = graph.traversal(). compute(graph -> graph.compute(SparkGraphComputer.class). workers(10). configure( "gremlin.spark.persistContext" , true )). bulk(BigIntegerBulk.class). withStrategies(LazyBarrierStrategy.instance(), EventStrategy.instance()). withoutStrategies(MatchPredicateStrategy.class, AdjacentIncidentStrategy.class). sack(ArrayList:: new , ArrayList::clone, (a,b) -> a.addAll(b)) ....for DSLs, it would be the same save: g = graph.traversal(SocialTraversal.class).....
          Hide
          okram Marko A. Rodriguez added a comment - - edited

          I think we can get rid of TraversalEngine. We basically just have:

          Optional<GraphComputer> Traversal.Admin.getComputer();
          

          Then this stuff:

          if (traversal.getEngine().isComputer()) { ... }
          

          simply becomes:

          if(traversal.getComputer().isPresent()) { ... }
          

          What is nice is that a ReasoningStrategy (for example) which determines whether to execute OLAP or OLTP, would simply traversal.setComputer(null) if it believes the traversal should be executed OLTP. Likewise, the ReasoningStrategy can augment the GraphComputer on the fly via:

          // ReasoningStrategy
          if(simpleButOLAP) {
            traversal.setComputer(traversal.getComputer().get().workers(1));
          } else if(superSuperSimpleThusOLTP) {
            traversal.setComputer(null);
          }
          // and check this out with our new computer.vertices().edges() filters in 3.2.0
          traversal.setComputer(traversal.getComputer().vertices(hasLabel("person")).edges(hasLabel("knows"));
          // BAM!!!!!!!!!!!!!!!!!!!!!!!!
          

          Thus, there is no ReasoningEngine (TraversalEngine goes away). Likewise, there is no GremlinServerEngine. For Gremlin Server execution, you could do:

          g = graph.traversal().withStrategy(GremlinServerStrategy.build().ip(127.0.0.2).port(134).create())
          

          Then you no longer need all this nasty infrastructure: https://issues.apache.org/jira/browse/TINKERPOP3-575. Instead, GremlinServerStrategy would simply append a GremlinServerResultStep at the end of the Traversal EXACTLY how ComputerResultStep is appended for graph computer execution.

          To make this crystal clear, we could remove TraversalSource.compute() and have it be this:

          g = graph.traversal().withStrategy(ComputerStrategy.build().compute(graph -> graph.compute(Spark).workers(10)).attachElements(false)).create())
          

          In essence, graph.traversal().compute(Function<Graph,GraphComputer>) is shorthand for the above. As it is right now ComputerStrategy simply adds a ComputerResultStep to the end of the Traversal (as a Finalization). However, with this model, it would need to be a Decoration.

          We could have a shorthand be:

          g = graph.traversal().withStrategy(ComputerStrategy.using(SparkGraphComputer))
          

          Or, using the default GraphComputer of the Graph

          g = graph.traversal().withStrategy(ComputerStrategy.instance())
          

          Ah... the plot thickens. Then this whole Traversal.Admin.getComputer() stuff can simply be this:

          public Optional<GraphComputer> getComputer() {
            if(this.strategies.toList().contains(ComputerStrategy))
              return strategies.toList().get(ComputerStrategy).getComputer()
          }
          

          That is some crazy talk but its genious in that EVERYTHING is a TraversalStrategy. Nothing is "special" – no engines, no computers. Its all via strategies.

          Show
          okram Marko A. Rodriguez added a comment - - edited I think we can get rid of TraversalEngine . We basically just have: Optional<GraphComputer> Traversal.Admin.getComputer(); Then this stuff: if (traversal.getEngine().isComputer()) { ... } simply becomes: if (traversal.getComputer().isPresent()) { ... } What is nice is that a ReasoningStrategy (for example) which determines whether to execute OLAP or OLTP, would simply traversal.setComputer(null) if it believes the traversal should be executed OLTP. Likewise, the ReasoningStrategy can augment the GraphComputer on the fly via: // ReasoningStrategy if (simpleButOLAP) { traversal.setComputer(traversal.getComputer().get().workers(1)); } else if (superSuperSimpleThusOLTP) { traversal.setComputer( null ); } // and check this out with our new computer.vertices().edges() filters in 3.2.0 traversal.setComputer(traversal.getComputer().vertices(hasLabel( "person" )).edges(hasLabel( "knows" )); // BAM!!!!!!!!!!!!!!!!!!!!!!!! Thus, there is no ReasoningEngine ( TraversalEngine goes away). Likewise, there is no GremlinServerEngine . For Gremlin Server execution, you could do: g = graph.traversal().withStrategy(GremlinServerStrategy.build().ip(127.0.0.2).port(134).create()) Then you no longer need all this nasty infrastructure: https://issues.apache.org/jira/browse/TINKERPOP3-575 . Instead, GremlinServerStrategy would simply append a GremlinServerResultStep at the end of the Traversal EXACTLY how ComputerResultStep is appended for graph computer execution. To make this crystal clear, we could remove TraversalSource.compute() and have it be this: g = graph.traversal().withStrategy(ComputerStrategy.build().compute(graph -> graph.compute(Spark).workers(10)).attachElements( false )).create()) In essence, graph.traversal().compute(Function<Graph,GraphComputer>) is shorthand for the above. As it is right now ComputerStrategy simply adds a ComputerResultStep to the end of the Traversal (as a Finalization ). However, with this model, it would need to be a Decoration . We could have a shorthand be: g = graph.traversal().withStrategy(ComputerStrategy.using(SparkGraphComputer)) Or, using the default GraphComputer of the Graph g = graph.traversal().withStrategy(ComputerStrategy.instance()) Ah... the plot thickens. Then this whole Traversal.Admin.getComputer() stuff can simply be this: public Optional<GraphComputer> getComputer() { if ( this .strategies.toList().contains(ComputerStrategy)) return strategies.toList().get(ComputerStrategy).getComputer() } That is some crazy talk but its genious in that EVERYTHING is a TraversalStrategy . Nothing is "special" – no engines, no computers. Its all via strategies.
          Hide
          okram Marko A. Rodriguez added a comment -

          Check this idea out:

          g = graph.traversal().withComputer(graph -> graph.compute(SparkGraphComputer.class).workers(10));
          g.V().out().values('name')
          

          This would compile to:

          [TraversalVertexProgramStep([GraphStep,VerticesStep,PropertiesStep]),ComputerResultStep]
          

          where,

          TraversalVertexProgramStep<Graph,ComputerResult>
          ComputerResultStep<ComputerResult,E>
          

          Next, check this:

          g = graph.traversal().withComputer(graph -> graph.compute(SparkGraphComputer.class).workers(10));
          g.V().hasLabel('person').pageRank(out('knows')).order().by('page.rank',decr)
          

          This will compile to:

          [TraversalVertexProgramStep([GraphStep,HasStep]),PageRankVertexProgramStep([VerticesStep]),TraversalVertexProgramStep([OrderStep]),ComputerResultStep]
          

          How does this work?!

          • TraversalVertexProgramStep will pass a ComputerResult to PageRankVertexProgram.
            • The ComputerResult.graph() will have HALTED_TRAVERSERS properties on all the person vertices (as that is what was computed by the vertex program).
          • PageRankVertexProgram will then pass a ComputerResult to the next TraversalVertexProgram.
            • That ComputerResult.graph() will have HALTED_TRAVERSER on all the person vertices and PAGE_RANK on all the vertices.
          • TraversalVertexProgram will then execute OrderStep which sorts the person-vertices with HALTED_TRAVERSERS on them by their page.rank properties computed previously.
          • ComputerResultStep will then take get the ComputerResult.memory.reducing and iterate it out.

          ComputerResultStep (like now) is smart about either pulling from the graph or from the sideEffect memory. What makes things different from now is that TraversalVertexProgramStep will come into existence and pull out the respective logic from ComputerResultStep. In this way, It will be possible to chain {XXXVertexProgramStep-steps and thus, have a traversal that is multiple OLAP jobs in sequence and thus, this ticket will fall naturally from this https://issues.apache.org/jira/browse/TINKERPOP-570. The whole trick to all of this is that (as we currently do) we save the state of the computation in the graph (and memory) and thus, feeding program into the next just takes over the computation. PageRankVertexProgram doesn't use HALTED_TRAVERSERS in its computation, so it just ignores it. However, TraversalVertexProgram can later access PAGE_RANK and thus, use the results of the previous OLAP computation.

          Finally, you want "lambda vertex programs?"

          g = graph.traversal().withComputer(graph -> graph.compute(SparkGraphComputer.class).workers(10));
          myProgram = MyVertexProgram.build().create();
          g.V().program(myProgram).values('myProgramCounters')
          

          which compiles to:

          [TraversalVertexProgramStep([GraphStep]),LambdaVertexProgramStep(MyVertexProgram),TraversalVertexProgramStep([PropertiesStep]),ComputerResultStep]
          

          Thus, just like filter(),flatMap(), etc....

          Show
          okram Marko A. Rodriguez added a comment - Check this idea out: g = graph.traversal().withComputer(graph -> graph.compute(SparkGraphComputer.class).workers(10)); g.V().out().values('name') This would compile to: [TraversalVertexProgramStep([GraphStep,VerticesStep,PropertiesStep]),ComputerResultStep] where, TraversalVertexProgramStep<Graph,ComputerResult> ComputerResultStep<ComputerResult,E> Next, check this: g = graph.traversal().withComputer(graph -> graph.compute(SparkGraphComputer.class).workers(10)); g.V().hasLabel('person').pageRank(out('knows')).order().by('page.rank',decr) This will compile to: [TraversalVertexProgramStep([GraphStep,HasStep]),PageRankVertexProgramStep([VerticesStep]),TraversalVertexProgramStep([OrderStep]),ComputerResultStep] How does this work?! TraversalVertexProgramStep will pass a ComputerResult to PageRankVertexProgram. The ComputerResult.graph() will have HALTED_TRAVERSERS properties on all the person vertices (as that is what was computed by the vertex program). PageRankVertexProgram will then pass a ComputerResult to the next TraversalVertexProgram. That ComputerResult.graph() will have HALTED_TRAVERSER on all the person vertices and PAGE_RANK on all the vertices. TraversalVertexProgram will then execute OrderStep which sorts the person-vertices with HALTED_TRAVERSERS on them by their page.rank properties computed previously. ComputerResultStep will then take get the ComputerResult.memory.reducing and iterate it out. ComputerResultStep (like now) is smart about either pulling from the graph or from the sideEffect memory. What makes things different from now is that TraversalVertexProgramStep will come into existence and pull out the respective logic from ComputerResultStep . In this way, It will be possible to chain { XXXVertexProgramStep -steps and thus, have a traversal that is multiple OLAP jobs in sequence and thus, this ticket will fall naturally from this https://issues.apache.org/jira/browse/TINKERPOP-570 . The whole trick to all of this is that (as we currently do) we save the state of the computation in the graph (and memory) and thus, feeding program into the next just takes over the computation. PageRankVertexProgram doesn't use HALTED_TRAVERSERS in its computation, so it just ignores it. However, TraversalVertexProgram can later access PAGE_RANK and thus, use the results of the previous OLAP computation. Finally, you want "lambda vertex programs?" g = graph.traversal().withComputer(graph -> graph.compute(SparkGraphComputer.class).workers(10)); myProgram = MyVertexProgram.build().create(); g.V().program(myProgram).values('myProgramCounters') which compiles to: [TraversalVertexProgramStep([GraphStep]),LambdaVertexProgramStep(MyVertexProgram),TraversalVertexProgramStep([PropertiesStep]),ComputerResultStep] Thus, just like filter() , flatMap() , etc....
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user okram opened a pull request:

          https://github.com/apache/incubator-tinkerpop/pull/215

          TINKERPOP-971: TraversalSource should be fluent like GraphComputer

          https://issues.apache.org/jira/browse/TINKERPOP-971

          The concept of a `TraversalSource` is now very explicit and used extensively. Any traversal is composed of both a `TraversalSource` and a `Traversal`. Prior to this PR, `TraversalSources` were manipulated via a very awkward/error prone "builder model." Moreover, there were objects that didn't need to exist – e.g. `TraversalEngine`. With this PR, the API now looks like this:

          ```
          g = graph.traversal().withCompute(SparkGraphComputer.class).withStrategies(ReadOnlyStrategy.instance())
          g.V().count()
          ```

          Note that `withComputer()` has lots of useful overloads.

          ```
          g = graph.traversal().withComputer(h -> h.compute(SparkGraphComputer.class).workers(10))
          ```

          This change is "lightly breaking." For users, nothing changes. The previous `Builder` model still exists in a `Deprecated` state. All graph system providers have to do is:

          • Implement `XXXGraphProvider.getGraphComputer()` (if and only if they support `GraphComputer`).
          • Change `traverser.getEngine().isGraphComputer()` to `traversal.getStrategies().onGraphComputer()` if they have custom `TraversalStrategy` implementations.
          • Change `implements EngineDependent` to `implements GraphComputing` if they have custom steps that implemented `EngineDependent`.

          CHANGELOG:

          ```

          • Refactored `TraversalSource` model to allow fluent-method construction of `TraversalSources`.
          • Deprecated the concept of a `TraversalSource.Builder`.
          • Removed the concept of a `TraversalEngine`. All `Traversal` modulations now mediated by `TraversalStrategies`. (breaking)
          • Added `SideEffectStrategy` for registering sideEffects in a spawned `Traversal`.
          • Added `SackStrategy` for registering a sack for a spawned `Traversal`.
          • Added `RequirementsStrategy` and `RequirementsStep` for adding dynamic `TraverserRequirements` to a `Traversal`.
          • Removed `EngineDependentStrategy`.
          • Renamed step interface `EngineDependent` to `GraphComputing` with method `onGraphComputer()`. (breaking)
          • Cleaned up various `TraversalStrategy` tests now that `TraversalEngine` no longer exists.
            ```

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-971

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/incubator-tinkerpop/pull/215.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 #215


          commit 5f2cd6770d56b69c3b0eebf07152315237837f12
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-02-09T00:35:46Z

          The most brutal refactor. There is no such thing as a TraversalSource.Builder anymore. All there are are TraversalSources. These are simply a wrapper of a TraversalStrategy and a Graph. There is no such thing as a TraversalEngine anymore. What was ComputerTraversalEngine is now simply a strategy of TraversalVertexProgramStrategy. There is no such thing as EngineDependentStrategy – this is simply TraversalVertexProgramStrategy. In short, all there are are Graphs and TraversalStrategies. So so so so so much cleaner. This WILL NOT be backwards compatible for graph system providers. They will need to update their GraphProvider implementations – two methods changed. Moreover, if they have an XXXStrategy that calls traversal.getEngine().isComputer(), they will need to change that to traversal.getStrategies().onGraphComputer(). Really simple to for them to fix. It took me less a minute to fix up Spark, Giraph, and Neo4j. These changes WILL be backwards compatible for users at the level of graph.traversal(computer(SparkGraphComputer)) still working (though Depcrecation introduced). The GraphTraversal.Builder will simply turn into a bunch of TravesalSource calls. It is not used outside of this context. This commit is NOT complete and will be completed on the next sprint. Moreover, there are lots of cleanups and JavaDocing that I need to do – I just don't want to lose this work so pushing.

          commit 908433faf631f6d585dd2ff662c430da030f2857
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-02-09T12:36:13Z

          added back in ComputerTraversalEngine, StandardTraversalEngine, and TraversalSource.Builder. All are deprecated but are functional with respects to Graph.traversal(TravesalSource.Builder). Both TinkerGraphProvider and SparkGraphProvider have a 50-percent chance of either using the new traversal source model or the old deprecated builder model. This way, we are certain that, for users, this push is backwards compatible. Note that for graph system providers, there is only one new method they need to implement in their XXXGraphProvider test code – getGraphComputer(). Thus, this ticket is extremely gentle on both users and providers even though it is a massive refactor.

          commit 229474ab17a20de9be93325f8d170d915f60414a
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-02-09T14:29:08Z

          Added lots of JavaDoc. Lots of organization and clean up. Ready for PR.

          commit 2db6faacff947a3ee456585714101d6ce0399f35
          Author: Marko A. Rodriguez <okrammarko@gmail.com>
          Date: 2016-02-09T14:42:14Z

          renamed EngineDependent to GraphComputing as there is no longer the concept of a TraversalEngine.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user okram opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/215 TINKERPOP-971 : TraversalSource should be fluent like GraphComputer https://issues.apache.org/jira/browse/TINKERPOP-971 The concept of a `TraversalSource` is now very explicit and used extensively. Any traversal is composed of both a `TraversalSource` and a `Traversal`. Prior to this PR, `TraversalSources` were manipulated via a very awkward/error prone "builder model." Moreover, there were objects that didn't need to exist – e.g. `TraversalEngine`. With this PR, the API now looks like this: ``` g = graph.traversal().withCompute(SparkGraphComputer.class).withStrategies(ReadOnlyStrategy.instance()) g.V().count() ``` Note that `withComputer()` has lots of useful overloads. ``` g = graph.traversal().withComputer(h -> h.compute(SparkGraphComputer.class).workers(10)) ``` This change is "lightly breaking." For users, nothing changes. The previous `Builder` model still exists in a `Deprecated` state. All graph system providers have to do is: Implement `XXXGraphProvider.getGraphComputer()` (if and only if they support `GraphComputer`). Change `traverser.getEngine().isGraphComputer()` to `traversal.getStrategies().onGraphComputer()` if they have custom `TraversalStrategy` implementations. Change `implements EngineDependent` to `implements GraphComputing` if they have custom steps that implemented `EngineDependent`. CHANGELOG: ``` Refactored `TraversalSource` model to allow fluent-method construction of `TraversalSources`. Deprecated the concept of a `TraversalSource.Builder`. Removed the concept of a `TraversalEngine`. All `Traversal` modulations now mediated by `TraversalStrategies`. ( breaking ) Added `SideEffectStrategy` for registering sideEffects in a spawned `Traversal`. Added `SackStrategy` for registering a sack for a spawned `Traversal`. Added `RequirementsStrategy` and `RequirementsStep` for adding dynamic `TraverserRequirements` to a `Traversal`. Removed `EngineDependentStrategy`. Renamed step interface `EngineDependent` to `GraphComputing` with method `onGraphComputer()`. ( breaking ) Cleaned up various `TraversalStrategy` tests now that `TraversalEngine` no longer exists. ``` You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-971 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/215.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 #215 commit 5f2cd6770d56b69c3b0eebf07152315237837f12 Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-02-09T00:35:46Z The most brutal refactor. There is no such thing as a TraversalSource.Builder anymore. All there are are TraversalSources. These are simply a wrapper of a TraversalStrategy and a Graph. There is no such thing as a TraversalEngine anymore. What was ComputerTraversalEngine is now simply a strategy of TraversalVertexProgramStrategy. There is no such thing as EngineDependentStrategy – this is simply TraversalVertexProgramStrategy. In short, all there are are Graphs and TraversalStrategies. So so so so so much cleaner. This WILL NOT be backwards compatible for graph system providers. They will need to update their GraphProvider implementations – two methods changed. Moreover, if they have an XXXStrategy that calls traversal.getEngine().isComputer(), they will need to change that to traversal.getStrategies().onGraphComputer(). Really simple to for them to fix. It took me less a minute to fix up Spark, Giraph, and Neo4j. These changes WILL be backwards compatible for users at the level of graph.traversal(computer(SparkGraphComputer)) still working (though Depcrecation introduced). The GraphTraversal.Builder will simply turn into a bunch of TravesalSource calls. It is not used outside of this context. This commit is NOT complete and will be completed on the next sprint. Moreover, there are lots of cleanups and JavaDocing that I need to do – I just don't want to lose this work so pushing. commit 908433faf631f6d585dd2ff662c430da030f2857 Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-02-09T12:36:13Z added back in ComputerTraversalEngine, StandardTraversalEngine, and TraversalSource.Builder. All are deprecated but are functional with respects to Graph.traversal(TravesalSource.Builder). Both TinkerGraphProvider and SparkGraphProvider have a 50-percent chance of either using the new traversal source model or the old deprecated builder model. This way, we are certain that, for users, this push is backwards compatible. Note that for graph system providers, there is only one new method they need to implement in their XXXGraphProvider test code – getGraphComputer(). Thus, this ticket is extremely gentle on both users and providers even though it is a massive refactor. commit 229474ab17a20de9be93325f8d170d915f60414a Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-02-09T14:29:08Z Added lots of JavaDoc. Lots of organization and clean up. Ready for PR. commit 2db6faacff947a3ee456585714101d6ce0399f35 Author: Marko A. Rodriguez <okrammarko@gmail.com> Date: 2016-02-09T14:42:14Z renamed EngineDependent to GraphComputing as there is no longer the concept of a TraversalEngine.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-181957720

          VOTE +1. `mvn clean install`, integration tested, and built/published docs.

          You can review the docs here: http://tinkerpop.apache.org/docs/3.2.0-SNAPSHOT/reference/

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-181957720 VOTE +1. `mvn clean install`, integration tested, and built/published docs. You can review the docs here: http://tinkerpop.apache.org/docs/3.2.0-SNAPSHOT/reference/
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182069459

          ```
          [INFO] ------------------------------------------------------------------------
          [INFO] BUILD SUCCESS
          [INFO] ------------------------------------------------------------------------
          [INFO] Total time: 3:16:47.194s
          [INFO] Finished at: Tue Feb 09 14:06:43 MST 2016
          [INFO] Final Memory: 96M/731M
          [INFO] ------------------------------------------------------------------------
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182069459 ``` [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ------------------------------------------------------------------------ [INFO] Total time: 3:16:47.194s [INFO] Finished at: Tue Feb 09 14:06:43 MST 2016 [INFO] Final Memory: 96M/731M [INFO] ------------------------------------------------------------------------ ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182636140

          Note that when TINKERPOP-1140 is completed, `traversal.getStrategies().onGraphComputer()` will be replaced by `TraversalHelper.onGraphComputer(traversal)`. Minor note as this is one of the breaking changes for providers.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182636140 Note that when TINKERPOP-1140 is completed, `traversal.getStrategies().onGraphComputer()` will be replaced by `TraversalHelper.onGraphComputer(traversal)`. Minor note as this is one of the breaking changes for providers.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182685148

          • `mvn clean install`: passed
          • integration tests (incl. Neo4j): passed

          VOTE: +1

          One negative note though: Strategies are mainly respsonsible for a significant performance gap between graph methods and their respective traversal methods. As we've seen in the past, `.stream()...` performs terrible compared to an old school `for` loop, hence I don't think we should use `.stream()` so frequently in new code. Better we get rid of `.stream()` altogether.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182685148 `mvn clean install`: passed integration tests (incl. Neo4j): passed VOTE: +1 One negative note though: Strategies are mainly respsonsible for a significant performance gap between graph methods and their respective traversal methods. As we've seen in the past, `.stream()...` performs terrible compared to an old school `for` loop, hence I don't think we should use `.stream()` so frequently in new code. Better we get rid of `.stream()` altogether.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182811160

          @dkuppitz – where in particular are you seeing the use of `stream()`?

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182811160 @dkuppitz – where in particular are you seeing the use of `stream()`?
          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182849111 New code: https://github.com/apache/incubator-tinkerpop/pull/215/files#diff-61ffd0f149055394746c1d94ccab52e3R65 https://github.com/apache/incubator-tinkerpop/pull/215/files#diff-bd7750c1c674aeb94e058006af320abfR50 https://github.com/apache/incubator-tinkerpop/pull/215/files#diff-61f8ab6e66876a272a1fa1f3556b3012R51 https://github.com/apache/incubator-tinkerpop/pull/215/files#diff-3f79b4cdbc46d5f73ad1864f7d4402a5R80 https://github.com/apache/incubator-tinkerpop/pull/215/files#diff-34c81b4d45231b961aa8f748f3f4a339R72 https://github.com/apache/incubator-tinkerpop/pull/215/files#diff-94d7c8c44b3975d2312b04c7f54e500cR47 https://github.com/apache/incubator-tinkerpop/pull/215/files#diff-b6bca124a8cddd1cd9a8c0b004cd1634R53 Old code, but touched by the commit: https://github.com/apache/incubator-tinkerpop/pull/215/files#diff-0f4959bc7926d8e5fbd6395e96f19502R76 At least some of these code lines were probably copied from somewhere else as part of the refactoring process. However, as said before, it would probably be better to get rid of `stream()` once we stumble across it in the code we're working on.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182853040

          A response to each item in your list.

          • This will go away when TINKERPOP-1140 is complete. This is a "staging method" for now.
          • This is not a critical performance line of code. This is during `TraversalSource` construction which is like saying "a database connection creation method uses stream."
          • Same as previous.
          • Same as previous – submitting a `GraphComputer` job is crazy costly relative to this simple `stream()`.
          • This will go away when TINKERPOP-1140 is complete. This is a "staging method" for now.
          • This is test suite setup code. Performance doesn't matter.
          • Same as previous.

          To your "old code" reference:

          • Yes, we should not use `stream()` here. This class needs to be blazing fast. I believe you and I are the author of this class and we should fix it. That is for another ticket.

          NOTE: We should also remove all the `stream()` work in `StarGraph` (there is a butt load).

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182853040 A response to each item in your list. This will go away when TINKERPOP-1140 is complete. This is a "staging method" for now. This is not a critical performance line of code. This is during `TraversalSource` construction which is like saying "a database connection creation method uses stream." Same as previous. Same as previous – submitting a `GraphComputer` job is crazy costly relative to this simple `stream()`. This will go away when TINKERPOP-1140 is complete. This is a "staging method" for now. This is test suite setup code. Performance doesn't matter. Same as previous. To your "old code" reference: Yes, we should not use `stream()` here. This class needs to be blazing fast. I believe you and I are the author of this class and we should fix it. That is for another ticket. NOTE: We should also remove all the `stream()` work in `StarGraph` (there is a butt load).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182948628

          Did a basic build with `mvn clean install` and reviewed docs - looks good:

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/215#issuecomment-182948628 Did a basic build with `mvn clean install` and reviewed docs - looks good: VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/incubator-tinkerpop/pull/215

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/incubator-tinkerpop/pull/215
          Hide
          okram Marko A. Rodriguez added a comment -

          Wow.

          Show
          okram Marko A. Rodriguez added a comment - Wow.

            People

            • Assignee:
              okram Marko A. Rodriguez
              Reporter:
              okram Marko A. Rodriguez
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development