You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by sp...@apache.org on 2023/04/18 17:43:56 UTC
[tinkerpop] branch TINKERPOP-2855 created (now e41dd35626)
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a change to branch TINKERPOP-2855
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
at e41dd35626 TINKERPOP-2855 - wip
This branch includes the following new commits:
new e41dd35626 TINKERPOP-2855 - wip
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[tinkerpop] 01/01: TINKERPOP-2855 - wip
Posted by sp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a commit to branch TINKERPOP-2855
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit e41dd35626d30890bdef11df8874363136e97a7d
Author: Stephen Mallette <st...@amazon.com>
AuthorDate: Tue Apr 18 13:43:21 2023 -0400
TINKERPOP-2855 - wip
---
CHANGELOG.asciidoc | 3 +
docs/src/upgrade/release-3.5.x.asciidoc | 19 ++++
.../remote/traversal/AbstractRemoteTraversal.java | 5 ++
.../gremlin/process/traversal/Traversal.java | 5 ++
.../traversal/lambda/AbstractLambdaTraversal.java | 5 ++
.../strategy/decoration/PartitionStrategy.java | 2 +-
.../process/traversal/util/DefaultTraversal.java | 100 ++++++++++-----------
.../process/traversal/util/EmptyTraversal.java | 5 ++
.../process/traversal/util/TraversalHelper.java | 14 ++-
.../gremlin/process/traversal/TraversalTest.java | 5 ++
.../process/computer/GraphComputerTest.java | 8 +-
.../tinkergraph/structure/TinkerGraphTest.java | 51 +----------
12 files changed, 116 insertions(+), 106 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index fa18b671b8..919549042c 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -36,6 +36,9 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
* Bumped to Groovy 2.5.21.
* Fixed bug in parsing of `math()` expressions where variables were not being identified if they contained a text associated with a function.
* Refactored `FilterRankingStrategy` to improve performance for deeply nested traversals.
+* Refactored strategy application to improve performance by avoiding some excessive recursion.
+* Added `Traversal.lock()` to provide an explicit way to finalize a traversal object.
+* Changed `PartitionStrategy` to get its `Graph` object from the root traversal.
* Added `AuthInfoProvider` interface and `NewDynamicAuth()` to gremlin-go for dynamic authentication support.
* Bumped to `snakeyaml` 2.0 to fix security vulnerability.
* Bumped to Apache `commons-configuration` 2.9.0 to fix security vulnerability.
diff --git a/docs/src/upgrade/release-3.5.x.asciidoc b/docs/src/upgrade/release-3.5.x.asciidoc
index cbf9371810..6b67515564 100644
--- a/docs/src/upgrade/release-3.5.x.asciidoc
+++ b/docs/src/upgrade/release-3.5.x.asciidoc
@@ -31,6 +31,23 @@ complete list of all the modifications that are part of this release.
=== Upgrading for Users
+=== Upgrading for Providers
+
+==== Graph System Providers
+
+===== TraversalStrategy Expectations
+
+Given some important performance improvements in this version, providers may need to make alterations to their
+`TraversalStrategy` implementations as certain assumptions about the data available to a strategy may have changed.
+If a `TraversalStrategy` implementation requires access to the `Graph` implementation, side-effects, and similar data
+stored on a `Traversal`, it is best to get that information from the root of the traversal hierarchy rather than from
+the current traversal that the strategy is executing on as the strategy application process no longer take the
+expensive step to propagate that data throughout the traversal in between each strategy application.
+
+[source,java]
+----
+Optional<Graph> graph = TraversalHelper.getRootTraversal(traversal).getGraph();
+----
== TinkerPop 3.5.5
@@ -104,6 +121,7 @@ more likely now to note blocking behavior when a connection cannot be obtained.
See: link:https://issues.apache.org/jira/browse/TINKERPOP-2813[TINKERPOP-2813]
==== Added User Agent to Gremlin drivers
+
Previously, a server does not distinguish amongst the different types of clients connecting to it. We have now added
user agent to web socket handshake in all the drivers, each with their own configuration to enable or disable user agent.
User agent is enabled by default for all drivers.
@@ -117,6 +135,7 @@ User agent is enabled by default for all drivers.
See: link:https://issues.apache.org/jira/browse/TINKERPOP-2480[TINKERPOP-2480]
==== Update to SSL Handshake Timeout Configuration
+
Previously, the java driver relies on the default 10 second SSL handshake timeout defined by Netty. We have removed
the default SSL handshake timeout. The SSL handshake timeout will instead be capped by setting `connectionSetupTimeoutMillis`.
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/AbstractRemoteTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/AbstractRemoteTraversal.java
index 363de6eea3..d4583d3981 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/AbstractRemoteTraversal.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/AbstractRemoteTraversal.java
@@ -127,6 +127,11 @@ public abstract class AbstractRemoteTraversal<S,E> implements RemoteTraversal<S,
throw new UnsupportedOperationException("Remote traversals do not support this method");
}
+ @Override
+ public void lock() {
+ throw new UnsupportedOperationException("Remote traversals do not support this method");
+ }
+
@Override
public Optional<Graph> getGraph() {
throw new UnsupportedOperationException("Remote traversals do not support this method");
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java
index da221d5ed6..db5f774c6a 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java
@@ -542,6 +542,11 @@ public interface Traversal<S, E> extends Iterator<E>, Serializable, Cloneable, A
*/
public boolean isLocked();
+ /**
+ * Lock the traversal and perform any final adjustments to it after strategy application.
+ */
+ public void lock();
+
/**
* Gets the {@link Graph} instance associated to this {@link Traversal}.
*/
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java
index ac8919fff5..9aef7dec31 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java
@@ -170,6 +170,11 @@ public abstract class AbstractLambdaTraversal<S, E> implements Traversal.Admin<S
return null == this.bypassTraversal || this.bypassTraversal.isLocked();
}
+ @Override
+ public void lock() {
+ if (this.bypassTraversal != null) bypassTraversal.lock();
+ }
+
/**
* Implementations of this class can never be a root-level traversal as they are specialized implementations
* intended to be child traversals by design.
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategy.java
index 47d03364e7..78b588b7aa 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategy.java
@@ -126,7 +126,7 @@ public final class PartitionStrategy extends AbstractTraversalStrategy<Traversal
// writes.
final Optional<Graph.Features.VertexFeatures> vertexFeatures;
if (includeMetaProperties) {
- final Graph graph = traversal.getGraph().orElseThrow(
+ final Graph graph = TraversalHelper.getRootTraversal(traversal).getGraph().orElseThrow(
() -> new IllegalStateException("PartitionStrategy does not work with anonymous Traversals when includeMetaProperties is enabled"));
final Graph.Features.VertexFeatures vf = graph.features().vertex();
final boolean supportsMetaProperties = vf.supportsMetaProperties();
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java
index 4f314dcf8b..8e261d1d62 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java
@@ -121,63 +121,20 @@ public class DefaultTraversal<S, E> implements Traversal.Admin<S, E> {
@Override
public void applyStrategies() throws IllegalStateException {
if (this.locked) throw Traversal.Exceptions.traversalIsLocked();
- TraversalHelper.reIdSteps(this.stepPosition, this);
- 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) {
- // prepare the traversal and all its children for strategy application
- TraversalHelper.applyTraversalRecursively(t -> {
- if (hasGraph) t.setGraph(this.graph);
- t.setStrategies(this.strategies);
- t.setSideEffects(this.sideEffects);
- }, this);
-
- // 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.
+ // apply strategies in order on a root traversal only or a traversal that is logically considered a root
+ // for GraphComputer as a child of VertexProgramStep.
+ if (isRoot() || this.getParent() instanceof VertexProgramStep) {
final Iterator<TraversalStrategy<?>> strategyIterator = this.strategies.iterator();
while (strategyIterator.hasNext()) {
final TraversalStrategy<?> strategy = strategyIterator.next();
- TraversalHelper.applyTraversalRecursively(t -> {
- strategy.apply(t);
-
- // after the strategy is applied, it may have modified the traversal where a new traversal object
- // was added. if the strategy didn't set the Graph object it could leave that new traversal in a
- // state where another strategy might fail if that dependency is not satisfied
- TraversalHelper.applyTraversalRecursively(i -> {
- if (hasGraph) i.setGraph(this.graph);
- }, this);
- }, this);
+ TraversalHelper.applyTraversalRecursively(strategy::apply, this);
}
-
- // don't need to re-apply strategies to "this" - leads to endless recursion in GraphComputer.
- TraversalHelper.applyTraversalRecursively(t -> {
- if (hasGraph) t.setGraph(this.graph);
- if(!(t.isRoot()) && t != this && !t.isLocked()) {
- t.setSideEffects(this.sideEffects);
- t.applyStrategies();
- }
- }, this);
}
-
- this.finalEndStep = this.getEndStep();
- // finalize requirements
- if (this.isRoot()) {
- resetTraverserRequirements();
- }
- this.locked = true;
+ // lock the traversal and its children for execution, finalizing the copy of any data from parent to child
+ // as needed
+ this.lock();
}
private void resetTraverserRequirements() {
@@ -337,6 +294,45 @@ public class DefaultTraversal<S, E> implements Traversal.Admin<S, E> {
return this.locked;
}
+ /**
+ * Allow the locked property of the traversal to be directly set by those who know what they are doing. Are you
+ * sure you know what you're doing?
+ */
+ public void setLocked(final boolean locked) {
+ this.locked = locked;
+ }
+
+ @Override
+ public void lock() {
+ TraversalHelper.reIdSteps(stepPosition, this);
+
+ // normalize strategies, side-effects, and Graph through children. obviously ignore the root traversal, since
+ // there is no parent to draw data from and ignore stuff wrapped for GraphComputer execution and the root
+ // from which to draw child data from is logically the current traversal.
+ if (!isRoot() && !(parent instanceof VertexProgramStep)) {
+ final TraversalParent parent = getParent();
+ final Traversal.Admin parentTraversal = parent.asStep().getTraversal().asAdmin();
+ this.setStrategies(parentTraversal.getStrategies());
+ this.setSideEffects(parentTraversal.getSideEffects());
+
+ // not sure why java is complaining about generics here that i have to do this??
+ if (parentTraversal.getGraph().isPresent())
+ this.setGraph((Graph) parentTraversal.getGraph().get());
+ }
+
+ this.finalEndStep = this.getEndStep();
+
+ if (this.isRoot()) {
+ resetTraverserRequirements();
+ }
+
+ // lock the parent before the children
+ this.locked = true;
+
+ // now lock all the children
+ TraversalHelper.applyTraversalRecursively(Admin::lock, this, true);
+ }
+
/**
* Determines if the traversal has been fully iterated and resources released.
*/
@@ -420,7 +416,7 @@ public class DefaultTraversal<S, E> implements Traversal.Admin<S, E> {
public void setGraph(final Graph graph) {
this.graph = graph;
}
-
+
@Override
public boolean equals(final Object other) {
return other != null && other.getClass().equals(this.getClass()) && this.equals(((Traversal.Admin) other));
@@ -435,4 +431,4 @@ public class DefaultTraversal<S, E> implements Traversal.Admin<S, E> {
}
return result;
}
-}
+}
\ No newline at end of file
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java
index d3f6215a5a..2b94b85276 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java
@@ -112,6 +112,11 @@ public final class EmptyTraversal<S, E> implements Traversal.Admin<S, E> {
return true;
}
+ @Override
+ public void lock() {
+ // nothing to do here as this type of traversal is always in a locked state
+ }
+
@Override
public TraverserGenerator getTraverserGenerator() {
return null;
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java
index 2099ff85b6..9625a56174 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java
@@ -488,7 +488,19 @@ public final class TraversalHelper {
* @param traversal the root traversal to start application
*/
public static void applyTraversalRecursively(final Consumer<Traversal.Admin<?, ?>> consumer, final Traversal.Admin<?, ?> traversal) {
- consumer.accept(traversal);
+ applyTraversalRecursively(consumer, traversal, false);
+ }
+
+ /**
+ * Apply the provider {@link Consumer} function to the provided {@link Traversal} and all of its children.
+ *
+ * @param consumer the function to apply to the each traversal in the tree
+ * @param traversal the root traversal to start application
+ */
+ public static void applyTraversalRecursively(final Consumer<Traversal.Admin<?, ?>> consumer, final Traversal.Admin<?, ?> traversal,
+ final boolean applyToChildrenOnly) {
+ if (!applyToChildrenOnly)
+ consumer.accept(traversal);
// we get accused of concurrentmodification if we try a for(Iterable)
final List<Step> steps = traversal.getSteps();
diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java
index 05b1f6a17d..b7c352e23a 100644
--- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java
+++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java
@@ -360,6 +360,11 @@ public class TraversalTest {
return false;
}
+ @Override
+ public void lock() {
+
+ }
+
@Override
public Optional<Graph> getGraph() {
return null;
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java
index 1cd5de5b93..ce039e8ea0 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java
@@ -2122,8 +2122,12 @@ public class GraphComputerTest extends AbstractGremlinProcessTest {
assertEquals(6, graph2.traversal().V().values(PageRankVertexProgram.PAGE_RANK).count().next().intValue());
assertEquals(24, graph2.traversal().V().values().count().next().intValue());
//
- final ComputerResult result3 = graph2.compute(graphProvider.getGraphComputer(graph2).getClass())
- .program(TraversalVertexProgram.build().traversal(g.V().groupCount("m").by(__.values(PageRankVertexProgram.PAGE_RANK).count()).label().asAdmin()).create(graph2)).persist(GraphComputer.Persist.EDGES).result(GraphComputer.ResultGraph.NEW).submit().get();
+ final ComputerResult result3 = graph2.compute(graphProvider.getGraphComputer(graph2).getClass()).
+ program(TraversalVertexProgram.build().traversal(
+ g.V().groupCount("m").
+ by(__.values(PageRankVertexProgram.PAGE_RANK).count()).
+ label().asAdmin()).create(graph2)).
+ persist(GraphComputer.Persist.EDGES).result(GraphComputer.ResultGraph.NEW).submit().get();
final Graph graph3 = result3.graph();
final Memory memory3 = result3.memory();
assertTrue(memory3.keys().contains("m"));
diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java
index d516d7e30b..145d4939f5 100644
--- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java
+++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java
@@ -872,7 +872,7 @@ public class TinkerGraphTest {
final GraphTraversalSource g = traversal().withEmbedded(graph).withStrategies(new TraversalStrategy.ProviderOptimizationStrategy() {
@Override
public void apply(final Traversal.Admin<?, ?> traversal) {
- final Graph graph = traversal.getGraph().get();
+ final Graph graph = TraversalHelper.getRootTraversal(traversal).getGraph().get();
graph.addVertex("person");
}
});
@@ -886,28 +886,6 @@ public class TinkerGraphTest {
assertEquals(3, traversal().withEmbedded(graph).V().hasLabel("person").count().next().intValue());
}
- /**
- * Basically just trying to validate through {@link CountStrategy} that a child traversal constructed there gets
- * its {@link Graph} instance set. By using {@link AssertGraphStrategy} an exception can get triggered in betweeen
- * strategy applications to validate that the {@code Graph} is assigned.
- */
- @Test
- public void shouldSetGraphBetweenStrategyApplicationsForNewChildTraversalsConstructedByStrategies() throws Exception {
- final GraphTraversalSource g = TinkerGraph.open().traversal();
- g.addV("node").property(T.id, 1).as("1")
- .addV("node").property(T.id, 2).as("2")
- .addV("node").property(T.id, 3).as("3")
- .addV("node").property(T.id, 4).as("4")
- .addE("child").from("1").to("2")
- .addE("child").from("2").to("3")
- .addE("child").from("4").to("3").iterate();
-
- // this just needs to not throw an exception for it to pass
- g.withStrategies(AssertGraphStrategy.instance()).V(3).repeat(__.inE("child").outV().simplePath())
- .until(__.or(__.inE().count().is(0), __.loops().is(P.eq(2))))
- .path().count().next();
- }
-
/**
* Coerces a {@code Color} to a {@link TinkerGraph} during serialization. Demonstrates how custom serializers
* can be developed that can coerce one value to another during serialization.
@@ -1019,31 +997,4 @@ public class TinkerGraphTest {
return false;
}
}
-
- /**
- * Validates that a {@link Graph} is assigned to each {@link Traversal} if it is expected.
- */
- public static class AssertGraphStrategy extends AbstractTraversalStrategy<TraversalStrategy.VerificationStrategy> implements TraversalStrategy.VerificationStrategy {
-
- public static final AssertGraphStrategy INSTANCE = new AssertGraphStrategy();
-
- private AssertGraphStrategy() {}
-
- @Override
- public void apply(final Traversal.Admin<?, ?> traversal) {
- if (!(traversal instanceof AbstractLambdaTraversal) && (!traversal.getGraph().isPresent()
- || traversal.getGraph().get().equals(EmptyGraph.instance())))
- throw new VerificationException("Graph object not set on Traversal", traversal);
- }
-
- @Override
- public Set<Class<? extends VerificationStrategy>> applyPost() {
- return Collections.singleton(ComputerVerificationStrategy.class);
- }
-
- public static AssertGraphStrategy instance() {
- return INSTANCE;
- }
-
- }
}