[GitHub] [tinkerpop] spmallette commented on a change in pull request #1211: TINKERPOP-1568 Changed Strategy Application Methodology

[GitBox <gi...@apache.org>]

spmallette commented on a change in pull request #1211:
URL: https://github.com/apache/tinkerpop/pull/1211#discussion_r633483080



##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java
##########
@@ -121,34 +121,54 @@ public TraverserGenerator getTraverserGenerator() {
     public void applyStrategies() throws IllegalStateException {
         if (this.locked) throw Traversal.Exceptions.traversalIsLocked();
         TraversalHelper.reIdSteps(this.stepPosition, this);
-        this.strategies.applyStrategies(this);
-        boolean hasGraph = null != this.graph;
-        for (int i = 0, j = this.steps.size(); i < j; i++) { // "foreach" can lead to ConcurrentModificationExceptions
-            final Step step = this.steps.get(i);
-            if (step instanceof TraversalParent) {
-                for (final Traversal.Admin<?, ?> globalChild : ((TraversalParent) step).getGlobalChildren()) {
-                    globalChild.setStrategies(this.strategies);
-                    globalChild.setSideEffects(this.sideEffects);
-                    if (hasGraph) globalChild.setGraph(this.graph);
-                    globalChild.applyStrategies();
-                }
-                for (final Traversal.Admin<?, ?> localChild : ((TraversalParent) step).getLocalChildren()) {
-                    localChild.setStrategies(this.strategies);
-                    localChild.setSideEffects(this.sideEffects);
-                    if (hasGraph) localChild.setGraph(this.graph);
-                    localChild.applyStrategies();
-                }
+        final boolean hasGraph = null != this.graph;
+
+        // we only want to apply strategies on the top-level step or if we got some graphcomputer stuff going on.
+        // seems like in that case, the "top-level" of the traversal is really held by the VertexProgramStep which
+        // needs to have strategies applied on "pure" copies of the traversal it is holding (i think). it further
+        // seems that we need three recursions over the traversal hierarchy to ensure everything "works", where
+        // strategy application requires top-level strategies and side-effects pushed into each child and then after
+        // application of the strategies we need to call applyStrategies() on all the children to ensure that their
+        // steps get reId'd and traverser requirements are set.
+        if (isRoot() || this.getParent() instanceof VertexProgramStep) {
+
+            // note that prior to applying strategies to children we used to set side-effects and strategies of all
+            // children to that of the parent. under this revised model of strategy application from TINKERPOP-1568
+            // it doesn't appear to be necessary to do that (at least from the perspective of the test suite). by,
+            // moving side-effect setting after actual recursive strategy application we save a loop and by
+            // consequence also fix a problem where strategies might reset something in sideeffects which seems to
+            // happen in TranslationStrategy.
+            final Iterator<TraversalStrategy<?>> strategyIterator = this.strategies.toIterator();
+            while (strategyIterator.hasNext()) {
+                final TraversalStrategy<?> strategy = strategyIterator.next();
+                TraversalHelper.applyTraversalRecursively(strategy::apply, this);

Review comment:
       It's been a long time since i looked at this code and as i go back to it now, i recall wanting to revisit it before release as there still is a bit of a tangle in it that i was never quite satisfied with. that revisiting didn't happen, but i'm not sure it would have raised this issue because we don't seem to have any test assertions to this effect. Other things that bothered me are slowing coming back to memory the more I stare at this. I don't imagine more breaking changes, just some additional refactoring.
   
   While not ideal, is there a possible workaround for you, where you could alter your strategy to get the `Graph` from the parent?  Maybe just do: `Traversal.Admin<?,?> root = TraversalHelper.getRootTraversal(traversal)` for now. I've tested this approach with a contrived strategy for TinkerGraph and it works in that case.
   
   https://issues.apache.org/jira/browse/TINKERPOP-2568




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org