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 2018/06/15 12:18:02 UTC
[12/50] tinkerpop git commit: TINKERPOP-1518 GraphProvider allows for
caching of graph features
TINKERPOP-1518 GraphProvider allows for caching of graph features
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo
Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/aec17098
Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/aec17098
Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/aec17098
Branch: refs/heads/TINKERPOP-1878
Commit: aec170982aa03d1a07248721cb045ba95e11188b
Parents: ae2f304
Author: Stephen Mallette <sp...@genoprime.com>
Authored: Thu Jun 7 20:15:53 2018 -0400
Committer: Stephen Mallette <sp...@genoprime.com>
Committed: Thu Jun 7 20:15:53 2018 -0400
----------------------------------------------------------------------
CHANGELOG.asciidoc | 1 +
docs/src/dev/provider/index.asciidoc | 4 ++
docs/src/upgrade/release-3.4.x.asciidoc | 20 ++++++
.../tinkerpop/gremlin/structure/Graph.java | 1 -
.../tinkerpop/gremlin/AbstractGremlinTest.java | 64 ++++++++++++++------
.../apache/tinkerpop/gremlin/GraphManager.java | 5 ++
.../apache/tinkerpop/gremlin/GraphProvider.java | 16 +++++
.../neo4j/AbstractNeo4jGraphProvider.java | 23 ++++++-
8 files changed, 114 insertions(+), 20 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/aec17098/CHANGELOG.asciidoc
----------------------------------------------------------------------
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index e886107..a846363 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -33,6 +33,7 @@ This release also includes changes from <<release-3-3-3, 3.3.3>>.
* `min()` and `max()` now support all types implementing `Comparable`.
* Change the `toString()` of `Path` to be standardized as other graph elements are.
* `hadoop-gremlin` no longer generates a test artifact.
+* Allowed `GraphProvider` to expose a cached `Graph.Feature` object so that the test suite could re-use them to speed test runs.
* Fixed a bug in `ReducingBarrierStep`, that returned the provided seed value despite no elements being available.
* Changed the order of `select()` scopes. The order is now: maps, side-effects, paths.
* Removed previously deprecated Credentials DSL infrastructure.
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/aec17098/docs/src/dev/provider/index.asciidoc
----------------------------------------------------------------------
diff --git a/docs/src/dev/provider/index.asciidoc b/docs/src/dev/provider/index.asciidoc
index f6a964b..896f85c 100644
--- a/docs/src/dev/provider/index.asciidoc
+++ b/docs/src/dev/provider/index.asciidoc
@@ -662,6 +662,10 @@ should validate that the ignored tests are appropriately bypassed and that there
definitions. Moreover, implementers should consider filling gaps in their own test suites, especially when
IO-related tests are being ignored.
+TIP: If it is expensive to construct a new `Graph` instance, consider implementing `GraphProvider.getStaticFeatures()`
+which can help by caching a static feature set for instances produced by that `GraphProvider` and allow the test suite
+to avoid that construction cost if the test is ignored.
+
The only test-class that requires any code investment is the `GraphProvider` implementation class. This class is a
used by the test suite to construct `Graph` configurations and instances and provides information about the
implementation itself. In most cases, it is best to simply extend `AbstractGraphProvider` as it provides many
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/aec17098/docs/src/upgrade/release-3.4.x.asciidoc
----------------------------------------------------------------------
diff --git a/docs/src/upgrade/release-3.4.x.asciidoc b/docs/src/upgrade/release-3.4.x.asciidoc
index 851d458..741fb3c 100644
--- a/docs/src/upgrade/release-3.4.x.asciidoc
+++ b/docs/src/upgrade/release-3.4.x.asciidoc
@@ -241,6 +241,26 @@ See: link:https://issues.apache.org/jira/browse/TINKERPOP-1522[TINKERPOP-1522]
==== Graph Database Providers
+===== Caching Graph Features
+
+For graph implementations that have expensive creation times, it can be time consuming to run the TinkerPop test suite
+as each test run requires a `Graph` instance even if the test is ultimately ignored becaue it doesn't pass the feature
+checks. To possibly help alleviate this problem, the `GraphProvider` interface now includes this method:
+
+[source,java]
+----
+public default Optional<Graph.Features> getStaticFeatures() {
+ return Optional.empty();
+}
+----
+
+This method can be implemented to return a cacheable set of features for a `Graph` generated from that `GraphProvider`.
+Assuming this method is faster than the cost of creating a new `Graph` instance, the test suite should execute
+significantly faster depending on how many tests end up being ignored.
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-1518[TINKERPOP-1518],
+link:
+
===== Configuring Interface
There were some changes to interfaces that were related to `Step`. A new `Configuring` interface was added that was
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/aec17098/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java
----------------------------------------------------------------------
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java
index 2fbfe03..dc14cc6 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java
@@ -40,7 +40,6 @@ import java.lang.reflect.InvocationTargetException;
import java.util.Collections;
import java.util.Iterator;
import java.util.Map;
-import java.util.NoSuchElementException;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/aec17098/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/AbstractGremlinTest.java
----------------------------------------------------------------------
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/AbstractGremlinTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/AbstractGremlinTest.java
index 25d7a55..8f55329 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/AbstractGremlinTest.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/AbstractGremlinTest.java
@@ -29,9 +29,9 @@ import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Vertex;
import org.apache.tinkerpop.gremlin.structure.VertexProperty;
import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.javatuples.Pair;
import org.junit.After;
import org.junit.Before;
-import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.rules.TestName;
import org.slf4j.Logger;
@@ -40,9 +40,11 @@ import org.slf4j.LoggerFactory;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
-import java.util.Random;
+import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.function.Consumer;
import java.util.stream.Collectors;
@@ -61,6 +63,9 @@ import static org.junit.Assume.assumeThat;
*/
public abstract class AbstractGremlinTest {
private static final Logger logger = LoggerFactory.getLogger(AbstractGremlinTest.class);
+
+ protected static final Map<Pair<Class<?>, String>, Boolean> featureCache = new HashMap<>();
+
protected Graph graph;
protected GraphTraversalSource g;
protected Configuration config;
@@ -75,8 +80,26 @@ public abstract class AbstractGremlinTest {
final LoadGraphWith[] loadGraphWiths = testMethod.getAnnotationsByType(LoadGraphWith.class);
final LoadGraphWith loadGraphWith = loadGraphWiths.length == 0 ? null : loadGraphWiths[0];
final LoadGraphWith.GraphData loadGraphWithData = null == loadGraphWith ? null : loadGraphWith.value();
+ final Set<FeatureRequirement> featureRequirementSet = getFeatureRequirementsForTest(testMethod, loadGraphWiths);
graphProvider = GraphManager.getGraphProvider();
+
+ final Optional<Graph.Features> staticFeatures = graphProvider.getStaticFeatures();
+ if (staticFeatures.isPresent()) {
+ for (FeatureRequirement fr : featureRequirementSet) {
+ try {
+ assumeThat(String.format("StaticFeatures of the graph do not support all of the features required by this test so it will be ignored: %s.%s=%s",
+ fr.featureClass().getSimpleName(), fr.feature(), fr.supported()),
+ staticFeatures.get().supports(fr.featureClass(), fr.feature()), is(fr.supported()));
+ } catch (NoSuchMethodException nsme) {
+ throw new NoSuchMethodException(String.format("[supports%s] is not a valid feature on %s", fr.feature(), fr.featureClass()));
+ } catch (UnsupportedOperationException uoe) {
+ // no worries - it just means that we can't use the cache to support this check and will have to
+ // incur the cost of instantiating a graph instance directly
+ }
+ }
+ }
+
graphProvider.getTestListener().ifPresent(l -> l.onTestStart(this.getClass(), name.getMethodName()));
config = graphProvider.standardGraphConfiguration(this.getClass(), name.getMethodName(), loadGraphWithData);
@@ -87,24 +110,11 @@ public abstract class AbstractGremlinTest {
graph = graphProvider.openTestGraph(config);
g = graphProvider.traversal(graph);
- // get feature requirements on the test method and add them to the list of ones to check
- final FeatureRequirement[] featureRequirement = testMethod.getAnnotationsByType(FeatureRequirement.class);
- final List<FeatureRequirement> frs = new ArrayList<>(Arrays.asList(featureRequirement));
-
- // if the graph is loading data then it will come with it's own requirements
- if (loadGraphWiths.length > 0) frs.addAll(loadGraphWiths[0].value().featuresRequired());
-
- // if the graph has a set of feature requirements bundled together then add those
- final FeatureRequirementSet[] featureRequirementSets = testMethod.getAnnotationsByType(FeatureRequirementSet.class);
- if (featureRequirementSets.length > 0)
- frs.addAll(Arrays.stream(featureRequirementSets)
- .flatMap(f -> f.value().featuresRequired().stream()).collect(Collectors.toList()));
-
- // process the unique set of feature requirements
- final Set<FeatureRequirement> featureRequirementSet = new HashSet<>(frs);
for (FeatureRequirement fr : featureRequirementSet) {
try {
- //System.out.println(String.format("Assume that %s meets Feature Requirement - %s - with %s", fr.featureClass().getSimpleName(), fr.feature(), fr.supported()));
+ // even if we checked static features above it's of little cost to recheck again with the real graph
+ // once it is instantiated. the real cost savings is preventing graph creation in the first place so
+ // let's double check that all is legit.
assumeThat(String.format("%s does not support all of the features required by this test so it will be ignored: %s.%s=%s",
graph.getClass().getSimpleName(), fr.featureClass().getSimpleName(), fr.feature(), fr.supported()),
graph.features().supports(fr.featureClass(), fr.feature()), is(fr.supported()));
@@ -263,6 +273,24 @@ public abstract class AbstractGremlinTest {
AbstractGremlinTest.verifyUniqueStepIds(traversal, 0, new HashSet<>());
}
+ private static Set<FeatureRequirement> getFeatureRequirementsForTest(final Method testMethod, final LoadGraphWith[] loadGraphWiths) {
+ // get feature requirements on the test method and add them to the list of ones to check
+ final FeatureRequirement[] featureRequirement = testMethod.getAnnotationsByType(FeatureRequirement.class);
+ final List<FeatureRequirement> frs = new ArrayList<>(Arrays.asList(featureRequirement));
+
+ // if the graph is loading data then it will come with it's own requirements
+ if (loadGraphWiths.length > 0) frs.addAll(loadGraphWiths[0].value().featuresRequired());
+
+ // if the graph has a set of feature requirements bundled together then add those
+ final FeatureRequirementSet[] featureRequirementSets = testMethod.getAnnotationsByType(FeatureRequirementSet.class);
+ if (featureRequirementSets.length > 0)
+ frs.addAll(Arrays.stream(featureRequirementSets)
+ .flatMap(f -> f.value().featuresRequired().stream()).collect(Collectors.toList()));
+
+ // process the unique set of feature requirements
+ return new HashSet<>(frs);
+ }
+
private static void verifyUniqueStepIds(final Traversal.Admin<?, ?> traversal, final int depth, final Set<String> ids) {
for (final Step step : traversal.asAdmin().getSteps()) {
/*for (int i = 0; i < depth; i++) System.out.print("\t");
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/aec17098/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/GraphManager.java
----------------------------------------------------------------------
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/GraphManager.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/GraphManager.java
index 6886465..7506226 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/GraphManager.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/GraphManager.java
@@ -183,6 +183,11 @@ public class GraphManager {
if (innerGraphProvider instanceof AutoCloseable)
((AutoCloseable) innerGraphProvider).close();
}
+
+ @Override
+ public Optional<Graph.Features> getStaticFeatures() {
+ return innerGraphProvider.getStaticFeatures();
+ }
}
}
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/aec17098/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/GraphProvider.java
----------------------------------------------------------------------
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/GraphProvider.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/GraphProvider.java
index c785cfc..1fcb147 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/GraphProvider.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/GraphProvider.java
@@ -294,6 +294,22 @@ public interface GraphProvider {
}
/**
+ * Gets a {@link Graph.Features} implementation that contains graph feature configuration that will never change
+ * from execution to execution of the tests given this current {@code GraphProvider} implementation. Implementing
+ * this method will allow the test suite to avoid creation of a {@link Graph} instance and thus speed up the
+ * execution of tests if that creation process is expensive. It is important that this static set of features be
+ * representative of what the {@link Graph} instance produced by this {@code GraphProvider} can actually do or
+ * else the cost of {@link Graph} instantiation will be incurred when it doesn't need to be. It is also important
+ * that this method be faster than the cost of {@link Graph} creation in the first place or there really won't be
+ * any difference in execution speed. In cases where some features are static and others are not, simply throw
+ * an {@code UnsupportedOperationException} from those features to let the test suite know that it cannot rely
+ * on them and the test suite will revert to using a constructed {@link Graph} instance.
+ */
+ public default Optional<Graph.Features> getStaticFeatures() {
+ return Optional.empty();
+ }
+
+ /**
* An annotation to be applied to a {@code GraphProvider} implementation that provides additional information
* about its intentions. The {@code Descriptor} is required by those {@code GraphProvider} implementations
* that will be assigned to test suites that use
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/aec17098/neo4j-gremlin/src/test/java/org/apache/tinkerpop/gremlin/neo4j/AbstractNeo4jGraphProvider.java
----------------------------------------------------------------------
diff --git a/neo4j-gremlin/src/test/java/org/apache/tinkerpop/gremlin/neo4j/AbstractNeo4jGraphProvider.java b/neo4j-gremlin/src/test/java/org/apache/tinkerpop/gremlin/neo4j/AbstractNeo4jGraphProvider.java
index 9226822..514ef2b 100644
--- a/neo4j-gremlin/src/test/java/org/apache/tinkerpop/gremlin/neo4j/AbstractNeo4jGraphProvider.java
+++ b/neo4j-gremlin/src/test/java/org/apache/tinkerpop/gremlin/neo4j/AbstractNeo4jGraphProvider.java
@@ -32,6 +32,7 @@ import org.apache.tinkerpop.gremlin.structure.Graph;
import java.io.File;
import java.util.HashSet;
+import java.util.Optional;
import java.util.Random;
import java.util.Set;
@@ -49,6 +50,26 @@ public abstract class AbstractNeo4jGraphProvider extends AbstractGraphProvider {
add(Neo4jVertexProperty.class);
}};
+ protected Graph.Features features = null;
+
+ @Override
+ public Graph openTestGraph(final Configuration config) {
+ final Graph graph = super.openTestGraph(config);
+
+ // we can just use the initial set of features taken from the first graph generated from the provider because
+ // neo4j feature won't ever change. don't think there is any danger of keeping this instance about even if
+ // the original graph instance goes out of scope.
+ if (null == features) {
+ this.features = graph.features();
+ }
+ return graph;
+ }
+
+ @Override
+ public Optional<Graph.Features> getStaticFeatures() {
+ return Optional.ofNullable(features);
+ }
+
@Override
public void clear(final Graph graph, final Configuration configuration) throws Exception {
if (null != graph) {
@@ -56,7 +77,7 @@ public abstract class AbstractNeo4jGraphProvider extends AbstractGraphProvider {
graph.close();
}
- if (configuration.containsKey(Neo4jGraph.CONFIG_DIRECTORY)) {
+ if (configuration != null && configuration.containsKey(Neo4jGraph.CONFIG_DIRECTORY)) {
// this is a non-in-sideEffects configuration so blow away the directory
final File graphDirectory = new File(configuration.getString(Neo4jGraph.CONFIG_DIRECTORY));
deleteDirectory(graphDirectory);