You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by dpitera <gi...@git.apache.org> on 2017/03/06 20:20:14 UTC
[GitHub] tinkerpop pull request #569: Tinkerpop 1438: GraphManager becomes a customiz...
GitHub user dpitera opened a pull request:
https://github.com/apache/tinkerpop/pull/569
Tinkerpop 1438: GraphManager becomes a customizable interface
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dpitera/tinkerpop TINKERPOP-1438
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/569.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 #569
----
commit 445bce2e255499014921a3239f711951864919c5
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-20T21:00:36Z
Ignore a tree test that's killing travis.
Will be fixed as part of TINKERPOP-1509 CTR
commit e152a5050ec3871a3217677c9ef539576d74b5f7
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-21T17:19:53Z
Merge branch 'tp32'
Conflicts:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapper.java
commit 1998536f83d2c45cc44760ee6328cfcaed1a1c32
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-21T17:21:13Z
Merge branch 'tp32'
commit b0102688e37565a503e99b99d62c90527c22a730
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-22T18:00:20Z
Merge branch 'tp32'
commit d6501d3284fed1ab2bea66dc302a4a183ac67d34
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-23T12:37:11Z
Merge branch 'tp32'
commit 55e1678ce2fea9960adc9205e778cd1344fadcf8
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-23T12:46:51Z
Added some missing imports to fix compile problems CTR
commit 6c09b722c920965b10f912af8eb9d27d08e1cc10
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-27T15:37:19Z
Merge branch 'tp32'
commit a086fbe96201b82cdc70aa395e208061cd0bf85c
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-28T13:37:03Z
Merge branch 'tp32'
commit 24ff9e879b7af17b0e20daf976005d806650f903
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-28T14:02:59Z
Merge branch 'tp32'
commit 59d0a6957b9b868d8c6ba0931108352732ee8275
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-29T10:39:48Z
Merge branch 'tp32'
commit a4bff0c34bb38f772035e608a878654763d7e097
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-29T11:29:28Z
Merge branch 'tp32'
commit 77bbb427cfd961ae2d5fa990f863efe10d5ce080
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-29T11:55:14Z
Merge branch 'tp32'
commit 3fc700fdc19a6cb44d57aecf457a00b8eba0346a
Author: Marko A. Rodriguez <ok...@gmail.com>
Date: 2017-01-03T14:43:41Z
merged Spark 2.0.0 work. Massive undertaking that provided us performance improvements. Thanks @dalaro and @yucx.
commit df67d7a413e3525db7c69167c729c0522bbb8445
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-14T16:50:38Z
TINKERPOP-1130 Structured the IO compatibility tests
commit fc7b4457d8e740dc17bedafdd569db52e016b619
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-14T22:41:30Z
TINKERPOP-1130 Implemented many additional IO tests.
Cleaned up inconsistencies in how static files are stored. Improved asserts.
commit 8a2241c2cb290a9ac6b4976967ef16bfff485fe6
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-15T16:55:16Z
TINKERPOP-1130 Completed tests for typed IO
Still need some asserts and found lots of inconsistencies that were handled in the Model class with Compatibility assignments.
commit a10910f7dcd97cac274534e24cf0c4973376795f
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-15T18:58:20Z
TINKERPOP-1130 Made the test data for metrics static.
This helps prevent the data from regenerating on every build.
commit 4549ed025fe7d1dac9ed486a085a3aaf936cbd07
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-15T19:40:00Z
TINKERPOP-1130 Improved asserts on Graph elements.
commit fb0bb0182f9fd9becf64b06c90a08988e6fc0f2e
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-16T11:56:55Z
TINKERPOP-1130 Changed scope of gremlin-test to "test"
commit ab206a728e441cfe643a30973172115e179a97d4
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-19T14:16:11Z
TINKERPOP-1130 Add gremlin-test back as compile scope.
It can't be test scope since non-test code uses commons-io which comes from gremlin-test. Shouldn't be any harm in including gremlin-test this way as this is a utility module that isn't deployed or anything.
commit 50d179d31ad03adc3d16b37f6261777898b82c22
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-21T20:35:23Z
TINKERPOP-1130 Get the basics of Gryo 3.0 in place.
commit 7503d33a6fb4df9e8e1471162a87456195ef6fe9
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-21T20:52:30Z
TINKERPOP-1130 Enabled testing of int/double for GraphSON
Thanks to fixes in tp32/master it's now possible to test these primitive data types.
commit 12b472d6c1153f20ccb8689fd8ff88feabfb1dee
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-22T18:39:10Z
TINKERPOP-1130 Enabled GraphSON serialization tests for enums
Fixes on tp32 related to enum serialization allowed these tests to start working.
commit cc5904d5d88049df541280b794bf7d8e694539b8
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-23T13:34:40Z
TINKERPOP-1130 Added test support for ConjunctiveP in Gryo
This is now possible given fixes on tp32/master.
commit ebebc9af65af4400b00b0483ffd3a97d02defcaa
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-27T12:47:09Z
TINKERPOP-1130 Add 3.2.4 data files for gremlin-io-test
commit 7be09ae9a64ec8890cfae4761409394069f50d15
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-27T15:44:59Z
TINKERPOP-1130 Fixed ByteBuffer compatibility with Gryo.
Required a fix to 3.2.4 and the tp32 branch for this to work completely.
commit 6088ead30b31d5c13de35fa95261d4bc4a11a8f1
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-27T17:19:03Z
TINKERPOP-1130 Enable more tests on gryo 1.0 and 3.3.0.
commit ad72c472e0bbdec299a8dda7b47b568ba9fc2b64
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-27T18:49:53Z
TINKERPOP-1130 Added "incompatibility notes"
Provides a way to explain why something is untested (which essentially means incompatible) in the Model.
commit baf1d1700fa57b652ff938c5cd737b11a28f0b96
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-29T11:58:03Z
TINKERPOP-1130 Added TinkerGraph support in gremlin-io-test
Fixes in tp32/master allowed this to suddenly be supported.
commit ef12395486c2753cb452afac563a688a316c5595
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-12-29T12:40:25Z
TINKERPOP-1130 Added more asserts for Element related tests
----
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
GitHub user dpitera reopened a pull request:
https://github.com/apache/tinkerpop/pull/569
TINKERPOP-1438: GraphManager becomes a customizable interface
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dpitera/tinkerpop TINKERPOP-1438
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/569.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 #569
----
commit 7b20ce78be5205bb9a0bec29435d77e0084dd647
Author: Benjamin Anderson <b...@banjiewen.net>
Date: 2016-08-20T05:33:16Z
Replace GraphManager with interface
This enabled settings-based customization of the GraphManager
implementation class, allowing implementors to customize the behavior of
the GraphManager.
commit f9f3010889851b126437d1bd8f98e0a3f99ac9ba
Author: dpitera <dp...@us.ibm.com>
Date: 2016-11-21T18:01:47Z
GraphManager support opening of specific graphs
This changeset allows an implementor to open a specific graph. One may
use this concept to implement a dynamic graph cache.
Furthermore, to ensure that rebindings work as intended, i.e. the list
of graphs returned to the HttpGremlinEndpointHandler, or "open graphs",
must include the to-be-rebound-graph. This changeset includes a change
to return these rebound graphs specifically.
Similar story as above for the WebSockets class, StandardOpProcessor.
Similar story for sessions, SessionOpProcessor.
Furthermore, the serializers at server boot only need to be aware of the
graphs defined in the settings object, so the relevant change here is in
AbstractChannelizer.
Furthermore:
To encourage a GraphManager implementation whose modification of the
Map<String, Graph> object aligns more closely with accepted "Getter and
Setter" design patterns, we update the adding of graphs to the
GraphManager Map by calling the new `addGraph(String, Graph)` rather
than publicly editting it with a call to `getGraphs().put(String,
Graph)`.
Also added `addTraversalSource(String, TraversalSource) for same
reasons.
Also, updated BasicGraphManager according to the new specifications.
commit 1119f811c1cc91b286e514365501a01beaeaeaec
Author: dpitera <dp...@us.ibm.com>
Date: 2017-03-15T19:31:45Z
Allow for custom graph instantiations/closings
This allows an implementor to supply a custom openGraph function to
return a newly instantiated graph object, and similarly do the same to
close a graph object, while doing so through the graphManager for
reference tracking.
commit 4cb5a771ac78187d438cb453d212787de5579e60
Author: dpitera <dp...@us.ibm.com>
Date: 2017-03-15T19:34:09Z
Update docs acc. to GraphManager changes
----
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
> Therefore, i am not sure who would have been able to make use of the graphManager themselves without using a modified version of tinkerpop.
You would be surprised by what folks manage to make use of in their code. If it's public, people will use it. :smile: anyway, i mostly wanted to be sure we don't introduce any compile time breaks when folks go to use 3.2.5.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Integration tests finished successfully:
```
[INFO] Reactor Summary:
[INFO]
[INFO] Apache TinkerPop .................................. SUCCESS [8:43.526s]
[INFO] Apache TinkerPop :: Gremlin Shaded ................ SUCCESS [21.090s]
[INFO] Apache TinkerPop :: Gremlin Core .................. SUCCESS [1:14.775s]
[INFO] Apache TinkerPop :: Gremlin Test .................. SUCCESS [12.833s]
[INFO] Apache TinkerPop :: Gremlin Groovy ................ SUCCESS [3:02.384s]
[INFO] Apache TinkerPop :: Gremlin Groovy Test ........... SUCCESS [6.146s]
[INFO] Apache TinkerPop :: TinkerGraph Gremlin ........... SUCCESS [4:27.224s]
[INFO] Apache TinkerPop :: Gremlin Benchmark ............. SUCCESS [6.937s]
[INFO] Apache TinkerPop :: Gremlin Driver ................ SUCCESS [12.051s]
[INFO] Apache TinkerPop :: Neo4j Gremlin ................. SUCCESS [23:50.255s]
[INFO] Apache TinkerPop :: Gremlin Server ................ SUCCESS [17:12.591s]
[INFO] Apache TinkerPop :: Gremlin Python ................ SUCCESS [49.683s]
[INFO] Apache TinkerPop :: Hadoop Gremlin ................ SUCCESS [9:23.200s]
[INFO] Apache TinkerPop :: Spark Gremlin ................. SUCCESS [21:27.122s]
[INFO] Apache TinkerPop :: Giraph Gremlin ................ SUCCESS [2:21:10.655s]
[INFO] Apache TinkerPop :: Gremlin Console ............... SUCCESS [3:01.607s]
[INFO] Apache TinkerPop :: Gremlin Archetype ............. SUCCESS [0.134s]
[INFO] Apache TinkerPop :: Archetype - TinkerGraph ....... SUCCESS [1:12.427s]
[INFO] Apache TinkerPop :: Archetype - Server ............ SUCCESS [12.415s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 3:56:48.949s
[INFO] Finished at: Thu Mar 09 19:35:00 UTC 2017
[INFO] Final Memory: 109M/701M
[INFO] ------------------------------------------------------------------------
```
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106480663
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Use {@link String} graphName to retrieve {@link Graph} from the {@link Map} tracking
+ * {@link Graph} references, close the {@link Graph}, and remove it from the {@link Map}.
+ */
+ public final void closeGraph(final String graphName) {
+ final Graph graph = getGraph(graphName);
+ graphs.remove(graphName);
+ closeGraph(graph);
+ }
+
+ /**
+ * Close {@link Graph} object.
+ */
+ public final void closeGraph(final Graph graph) {
+ try {
+ graph.close();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
--- End diff --
At the expense of making things more confusing, imagine an implementor of Tinkerpop was building a graph database, called `G` for ease.
G promises horizontal scaling and guarantees linearizablity.
Therefore, G's graph objects must be consistent across multiple nodes.
Therefore, G will need to be able to instantiate its graph objects, and "close" its graph object's in a custom way-- that is-- one that guarantees linearizable consistency across nodes.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
This is merged - no problems getting it to master.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106460712
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Use {@link String} graphName to retrieve {@link Graph} from the {@link Map} tracking
+ * {@link Graph} references, close the {@link Graph}, and remove it from the {@link Map}.
+ */
+ public final void closeGraph(final String graphName) {
+ final Graph graph = getGraph(graphName);
+ graphs.remove(graphName);
+ closeGraph(graph);
+ }
+
+ /**
+ * Close {@link Graph} object.
+ */
+ public final void closeGraph(final Graph graph) {
+ try {
+ graph.close();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
--- End diff --
I'd suggest you change the signature:
```java
public Graph removeGraph(String graphName)
```
Then the caller can make it's own choice as to what to do with the removed instance.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r107671911
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,98 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ public Map<String, Graph> getGraphs();
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public Graph getGraph(final String gName);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>} returned by call to getGraphs()
+ */
+ public void addGraph(final String gName, final Graph g);
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
+
+ /**
+ * Get {@link TraversalSource} instance whose name matches {@link tsName}
+ *
+ * @return {@link TraversalSource} if exists, else null
+ */
+ public TraversalSource getTraversalSource(final String tsName);
/**
* Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
*/
- public Bindings getAsBindings() {
- final Bindings bindings = new SimpleBindings();
- graphs.forEach(bindings::put);
- traversalSources.forEach(bindings::put);
- return bindings;
- }
+
+ /**
+ * Add {@link TraversalSource} ts with name {@link String} tsName to
+ * {@link Map<String, TraversalSource>} returned by call to getTraversalSources()
+ */
+ public void addTraversalSource(final String tsName, final TraversalSource ts);
+
+ public Bindings getAsBindings();
/**
* Rollback transactions across all {@link Graph} objects.
*/
- public void rollbackAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().rollback();
- });
- }
+ public void rollbackAll();
/**
* Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
*/
- public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
- }
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn);
/**
* Commit transactions across all {@link Graph} objects.
*/
- public void commitAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().commit();
- });
- }
+ public void commitAll();
/**
* Selectively commit transactions on the specified graphs or the graphs of traversal sources.
*/
- public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
- }
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn);
+
+ /**
+ * Implementation that allows for custom graph-opening implementations; if the {@link Map}
+ * tracking graph references has a {@link Graph} object corresponding to the {@link String} graphName,
+ * then we return that {@link Graph}-- otherwise, we use the custom {@link Supplier} to instantiate a
+ * a new {@link Graph}, add it to the {@link Map} tracking graph references, and return said {@link Graph}.
+ */
+ public Graph openGraph(final String graphName, final Supplier<Graph> supplier);
--- End diff --
I'm feeling like the second argument here could offer a little more flexibility if it were `BiSupplier<String,Graph>` where the first argument was the `graphName` passed back in on itself (or ultimately whatever name the `GraphManager` implementation used to key the created `Graph` instance. That could be a useful configuration parameter to something like Neo4j where it could use the graph name to uniquely create a data directory for example.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108182233
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,98 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ public Map<String, Graph> getGraphs();
--- End diff --
ok - thanks for being patient on this. i knew this was going to be a gnarly pull request. it's one of the reasons i've sorta left this issue alone.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106456922
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Use {@link String} graphName to retrieve {@link Graph} from the {@link Map} tracking
+ * {@link Graph} references, close the {@link Graph}, and remove it from the {@link Map}.
+ */
+ public final void closeGraph(final String graphName) {
+ final Graph graph = getGraph(graphName);
+ graphs.remove(graphName);
+ closeGraph(graph);
+ }
+
+ /**
+ * Close {@link Graph} object.
+ */
+ public final void closeGraph(final Graph graph) {
+ try {
+ graph.close();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
--- End diff --
Throwing a `RuntimeException` here would bomb out the server? I don't see why I'd call this method on `DefaultGraphManager` if it doesn't do anything else besides call `graph.close()`.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Congrats on doing your first full integration test! Hope you stick around to help out on TinkerPop.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
I'm going to do some additional testing with my own `GraphManager` implementation just to see how it all feels. Anyway, I think this is firming up pretty nicely - getting pretty close to being able to call for a final review.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/569
deferring the merge to you @spmallette since you had taken the lead on the review, but i'm happy to handle 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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
I have updated my PR with both of your guys' comments, and I have mailed the dev list.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
I think I'm done with this round of comments. I suspect I might have a few more yet about the `GraphManager` interface itself - still rolling some ideas around in my mind on 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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108176975
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,98 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ public Map<String, Graph> getGraphs();
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public Graph getGraph(final String gName);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>} returned by call to getGraphs()
+ */
+ public void addGraph(final String gName, final Graph g);
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
+
+ /**
+ * Get {@link TraversalSource} instance whose name matches {@link tsName}
+ *
+ * @return {@link TraversalSource} if exists, else null
+ */
+ public TraversalSource getTraversalSource(final String tsName);
/**
* Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
*/
- public Bindings getAsBindings() {
- final Bindings bindings = new SimpleBindings();
- graphs.forEach(bindings::put);
- traversalSources.forEach(bindings::put);
- return bindings;
- }
+
+ /**
+ * Add {@link TraversalSource} ts with name {@link String} tsName to
+ * {@link Map<String, TraversalSource>} returned by call to getTraversalSources()
+ */
+ public void addTraversalSource(final String tsName, final TraversalSource ts);
+
+ public Bindings getAsBindings();
/**
* Rollback transactions across all {@link Graph} objects.
*/
- public void rollbackAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().rollback();
- });
- }
+ public void rollbackAll();
/**
* Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
*/
- public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
- }
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn);
/**
* Commit transactions across all {@link Graph} objects.
*/
- public void commitAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().commit();
- });
- }
+ public void commitAll();
/**
* Selectively commit transactions on the specified graphs or the graphs of traversal sources.
*/
- public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
- }
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn);
+
+ /**
+ * Implementation that allows for custom graph-opening implementations; if the {@link Map}
+ * tracking graph references has a {@link Graph} object corresponding to the {@link String} graphName,
+ * then we return that {@link Graph}-- otherwise, we use the custom {@link Supplier} to instantiate a
+ * a new {@link Graph}, add it to the {@link Map} tracking graph references, and return said {@link Graph}.
+ */
+ public Graph openGraph(final String graphName, final Supplier<Graph> supplier);
+
+ /**
+ * Implementation that allows for custom graph-closing implementations; it is up to the implementor
+ * to decide if this method should also remove the {@link Graph} graph from the {@link Map}
+ * tracking {@link Graph} references (and how it would do so).
+ */
+ public void closeGraph(final Graph graph) throws Exception;
--- End diff --
> Under what circumstance would an interacting component call closeGraph() but not want the Graph removed from the registry?
I mean sometimes things that _exist but are marked as "not usable"_ mean something other than _this doesn't exist. If this concept were important to a specific implementation I am sure they would have an `graph.unclose()` and then their `openGraph(String graphName)` would `unclose()` the graph that already exists in the reference tracker
> Piggy-backing on that last point, if you go down to DefaultGraphManager, I think that it should look to remove the instance from the registry.
I agree, but if we do not have the String representation of the graphName here then how can we do that?
> I still think this method is really just an overload of removeGraph(String) and should be named as such
I can live with that
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Pushed changes according to previous round of comments. Please re-review whenever you get a chance.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108893957
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,101 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Function;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * @Deprecated This returns a {@link Map} that should be immutable. Please refer to
+ * getGraphNames() for replacement.
+ *
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ @Deprecated
+ public Map<String, Graph> getGraphs();
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get a {@link Set} of {@link String} graphNames corresponding to names stored in the graph's
+ * reference tracker.
+ */
+ public Set<String> getGraphNames();
+ /**
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
+ */
+ public Graph getGraph(final String gName);
+
+ /**
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>}
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public void addGraph(final String gName, final Graph g);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
--- End diff --
I think you just need to deprecate `getTraversalSources()` and include `removeTraversalSource(String tsName)` and `getTraversalSourceNames()`. That should do it. I could do it after merge, but this body of work would be more complete if it were included as part of this PR.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106425058
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,201 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public Graph getGraph(String gName) {
+ return graphs.get(gName);
+ }
+
+ public void addGraph(String gName, Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public TraversalSource getTraversalSource(String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public void addTraversalSource(String tsName, TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * Basic graphManager has basic openGraph function.
+ */
+ public Graph openGraph(String graphName, Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ return supplier.get();
+ }
+
+ /**
+ * Close graph
+ */
+ public void closeGraph(Graph graph) {
+ try {
+ graph.close();
--- End diff --
> "closed" does that mean it should still be in the available set of graphs?
I would imagine not for sure. I didn't have access to the graphName for reasons described [here](https://github.com/apache/tinkerpop/pull/569#discussion_r106424327).
I think going forward the best thing to do is to make the `DefaultGraphManager` implement an actual `closeGraph(String graphName)` and `noop` `closeGraph(Graph graph)`. I would like to leave the interface `closeGraph(Graph graph)` though
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/569
@spmallette isn't a bot ;)
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106410916
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java ---
@@ -100,9 +101,28 @@ public ServerGremlinExecutor(final Settings settings, final ExecutorService grem
*/
public ServerGremlinExecutor(final Settings settings, final ExecutorService gremlinExecutorService,
final T scheduledExecutorService, final Class<T> scheduleExecutorServiceClass,
- final GraphManager graphManager) {
+ GraphManager graphManager) {
this.settings = settings;
+ if (null == graphManager) {
--- End diff --
looks like there's some indentation problems in the following lines of code. should be four spaces between the curly brackets.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
> and I have mailed the dev list.
scratch that-- failed to send lol
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108197184
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,98 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ public Map<String, Graph> getGraphs();
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public Graph getGraph(final String gName);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>} returned by call to getGraphs()
+ */
+ public void addGraph(final String gName, final Graph g);
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
+
+ /**
+ * Get {@link TraversalSource} instance whose name matches {@link tsName}
+ *
+ * @return {@link TraversalSource} if exists, else null
+ */
+ public TraversalSource getTraversalSource(final String tsName);
/**
* Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
*/
- public Bindings getAsBindings() {
- final Bindings bindings = new SimpleBindings();
- graphs.forEach(bindings::put);
- traversalSources.forEach(bindings::put);
- return bindings;
- }
+
+ /**
+ * Add {@link TraversalSource} ts with name {@link String} tsName to
+ * {@link Map<String, TraversalSource>} returned by call to getTraversalSources()
+ */
+ public void addTraversalSource(final String tsName, final TraversalSource ts);
+
+ public Bindings getAsBindings();
/**
* Rollback transactions across all {@link Graph} objects.
*/
- public void rollbackAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().rollback();
- });
- }
+ public void rollbackAll();
/**
* Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
*/
- public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
- }
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn);
/**
* Commit transactions across all {@link Graph} objects.
*/
- public void commitAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().commit();
- });
- }
+ public void commitAll();
/**
* Selectively commit transactions on the specified graphs or the graphs of traversal sources.
*/
- public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
- }
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn);
+
+ /**
+ * Implementation that allows for custom graph-opening implementations; if the {@link Map}
+ * tracking graph references has a {@link Graph} object corresponding to the {@link String} graphName,
+ * then we return that {@link Graph}-- otherwise, we use the custom {@link Supplier} to instantiate a
+ * a new {@link Graph}, add it to the {@link Map} tracking graph references, and return said {@link Graph}.
+ */
+ public Graph openGraph(final String graphName, final Supplier<Graph> supplier);
+
+ /**
+ * Implementation that allows for custom graph-closing implementations; it is up to the implementor
+ * to decide if this method should also remove the {@link Graph} graph from the {@link Map}
+ * tracking {@link Graph} references (and how it would do so).
+ */
+ public void closeGraph(final Graph graph) throws Exception;
--- End diff --
> Sorry I'm struggling to understand the need for this method.
Honestly I guess it's not necessary as you can do the same thing with `removeGraph(String)` by first retrieving the `Graph`. I just thought it made sense from a design perspective to `create` using a `String` and `destroy` the object itself.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106428624
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,87 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
-
+ public Map<String, Graph> getGraphs();
+
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
+ */
+ public Graph getGraph(String gName);
+
+ /**
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>} returned by call to getGraphs()
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public void addGraph(String gName, Graph g);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
/**
+ * Get {@link TraversalSource} instance whose name matches {@link tsName}
+ *
+ * @return {@link TraversalSource} if exists, else null
+ */
+
+ public TraversalSource getTraversalSource(String tsName);
+ /**
* Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
*/
- public Bindings getAsBindings() {
- final Bindings bindings = new SimpleBindings();
- graphs.forEach(bindings::put);
- traversalSources.forEach(bindings::put);
- return bindings;
- }
+
+ /**
+ * Add {@link TraversalSource} ts with name {@link String} tsName to
+ * {@link Map<String, TraversalSource>} returned by call to getTraversalSources()
+ */
+ public void addTraversalSource(String tsName, TraversalSource ts);
+
+ public Bindings getAsBindings();
/**
* Rollback transactions across all {@link Graph} objects.
*/
- public void rollbackAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().rollback();
- });
- }
+ public void rollbackAll();
/**
* Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
*/
- public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
- }
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn);
/**
* Commit transactions across all {@link Graph} objects.
*/
- public void commitAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().commit();
- });
- }
+ public void commitAll();
/**
* Selectively commit transactions on the specified graphs or the graphs of traversal sources.
*/
- public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
- }
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn);
/**
- * Selectively close transactions on the specified graphs or the graphs of traversal sources.
+ * Implementation that allows for custom graph-opening implementations.
*/
- private void closeTx(final Set<String> graphSourceNamesToCloseTxOn, final Transaction.Status tx) {
- final Set<Graph> graphsToCloseTxOn = new HashSet<>();
-
- // by the time this method has been called, it should be validated that the source/graph is present.
- // might be possible that it could have been removed dynamically, but that i'm not sure how one would do
- // that as of right now unless they were embedded in which case they'd need to know what they were doing
- // anyway
- graphSourceNamesToCloseTxOn.forEach(r -> {
- if (graphs.containsKey(r))
- graphsToCloseTxOn.add(graphs.get(r));
- else
- graphsToCloseTxOn.add(traversalSources.get(r).getGraph());
- });
+ public Graph openGraph(String graphName, Supplier<Graph> supplier);
- graphsToCloseTxOn.forEach(graph -> {
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen()) {
- if (tx == Transaction.Status.COMMIT)
- graph.tx().commit();
- else
- graph.tx().rollback();
- }
- });
- }
-}
\ No newline at end of file
+ /**
+ * Implementation that allows for custom graph-closing implementations.
+ */
+ public void closeGraph(Graph graph);
--- End diff --
oh, i see what you're saying. i think that if you add both, you should make a default implementation on the one with the String argument to use `getGraph()` and then pass that to the `closeGraph(Graph graph)` method.
was there a reason you went with `closeGraph()` as opposed to `removeGraph()`. it seems like the `GraphManager` is about managing `Graph` instances that it holds and "remove" is a better term for the behavior `GraphManager` is supposed to be doing there. @pluradj i think you'd suggested this addition - perhaps you could chime in on this.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r107668046
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,98 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ public Map<String, Graph> getGraphs();
--- End diff --
So, now we have `getGraphs()` which returns the `Map` of graph names and `Graph` instances but then we also expose get/put/remove operations that just manipulate that same `Map`. That seems like it needs more thought. In other words, why do:
```java
manager.addGraph("graph", new TinkerGraph());
manager.getGraph("graph");
```
if I can also do:
```java
manager.getGraphs().put("graph",new TinkerGraph());
manager.getGraphs().get("graph");
```
By offering two approaches to do the same thing we open the possibility to error. If a `GraphManager` implementation adds logic to `addGraph()` (or related method) then that logic can simply be bypassed by directly manipulating the `Map` via `getGraphs()`.
Seems like we would want to use `GraphManager` to enforce the logic of the implementation, thus `getGraphs()` should go away? Perhaps to lessen the breaking change for 3.2.x it is simply deprecated with some additional javadoc that states that the method is expected to return an umodifiable `Map`? replace that method with `Set<String> getGraphNames()` so you could iterate over that to get all the `Graph` instances or maybe have `GraphManager` implement `Iterable` so you could `foreach` and get an `iterator()`? and then finally replace all internal usage of `getGraphs()` so that it can safely be removed on the master branch.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r107668704
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,210 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Remove {@link Graph} corresponding to {@link String} graphName from {@link Map}
+ * tracking graph references.
+ */
+ public final void removeGraph(final String graphName) {
--- End diff --
Looking at this signature here, I think it would be better to mimic `Map.remove(k)` and return `Graph` as in:
```java
public final Graph removeGraph(final String graphName)
```
Then a caller would then just do:
```java
manager.removeGraph("graph").close();
```
or whatever they would do with their dynamically added graph.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
> Does this change introduce any compile-time breaking changes for anyone who has depended on the GraphManager class (or other things you've modified)?
I believe the answer is no (in the non-mutated use of pure tinkerpop) because:
1. The previous implementation's graphManager instantiation could only be retrieved by two places:
1. The [ServerGremlinExecutor](https://github.com/apache/tinkerpop/blob/ad51fb24eb4d621763f6b3c02029daa54e7dc20d/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java#L202) which is only accessible through the [GremlinServer](https://github.com/apache/tinkerpop/blob/c5b9680e70d696d000b945abbd72a3b8a3f97256/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GremlinServer.java#L328) which users do not have access to the instantiation unless they have modified their implementor's logic to define its own "GremlinServer" and not use Tinkerpop's
2. The [Context](https://github.com/apache/tinkerpop/blob/4293eb333dfbf3aea19cd326f9f3d13619ac0b54/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Context.java#L80) which also is instantiated by the same graphManager pulled from the servergremlinexecutor, and the Context class is final (and thus cannot be extended) and there is no way to retrieve that `Context` in the code.
Therefore, i am not sure who would have been able to make use of the graphManager themselves without using a modified version of tinkerpop.
> Can you elaborate on the usage of openGraph()
The `addGraph()` and `getGraph()` are to act as modifiers to the object holding the graph references, while `opengraph()` allows for a mechanism by which to instantiate a graph dynamically through the graphManager (and thus allow for reference tracking of it) that is not currently in said graphManager's reference tracking object. The use of a `Supplier` here allows for the implementor to define his/her own custom function by which to instantiate the graph
> If it stays on tp32 I may have to ask you to provide a second pull request to master that can also be evaluated as part of a second review/vote.
I would say once we get this one merged, then we can open the PR on master, but I would like to get it into the next release as well
> it will likely take a some time and iterations to get through before we can get it merged
Sounds good. I usually prefer quality to expediency. When I get around to fixing up some of your requests, I will leave a comment here saying I have pushed new code.
Thanks for your review. Let me know if you have more questions!
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108788582
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,101 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Function;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * @Deprecated This returns a {@link Map} that should be immutable. Please refer to
+ * getGraphNames() for replacement.
+ *
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ @Deprecated
+ public Map<String, Graph> getGraphs();
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get a {@link Set} of {@link String} graphNames corresponding to names stored in the graph's
+ * reference tracker.
+ */
+ public Set<String> getGraphNames();
+ /**
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
+ */
+ public Graph getGraph(final String gName);
+
+ /**
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>}
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public void addGraph(final String gName, final Graph g);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
--- End diff --
It occurred to me while implementing `GraphManager` that `TraversalSource` objects should also be managed by the interface. At a minimum, that would mean similar methods to what we have for `Graph`.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108175970
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,210 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Remove {@link Graph} corresponding to {@link String} graphName from {@link Map}
+ * tracking graph references.
+ */
+ public final void removeGraph(final String graphName) {
--- End diff --
Sounds good to me
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106464105
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Use {@link String} graphName to retrieve {@link Graph} from the {@link Map} tracking
+ * {@link Graph} references, close the {@link Graph}, and remove it from the {@link Map}.
+ */
+ public final void closeGraph(final String graphName) {
+ final Graph graph = getGraph(graphName);
+ graphs.remove(graphName);
+ closeGraph(graph);
+ }
+
+ /**
+ * Close {@link Graph} object.
+ */
+ public final void closeGraph(final Graph graph) {
+ try {
+ graph.close();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
--- End diff --
OK the more I think about the nuances here, I think you are right-- there should exist a `removeGraph(String)` and `closeGraph(Graph)`. The former is the antithesis to `addGraph()` while the latter is the antithesis to `openGraph()`.
Even though the `DefaultGraphManager.closeGraph()` will most likely not be called, I think it still makes sense as an example implementation to have it call `graph.close()`. Having said that, how shall I handle the `Exception`?
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Nice to see that all the tests are passing. Thanks for doing that.
I've given this a quick review and have a few top-level comments:
* Note that our code style is to explicitly use `final` for all variable declarations that require it - i think we make exceptions on exceptions in catch blocks and the var declaration of a `for()` loop, but that's about it. I think you've missed a few of those.
* Could you please rename `BasicGraphManager` to `DefaultGraphManager`? We only use the term "Basic" in one other class naming, so it's a bit of an anomaly compared to "Default".
* Does this change introduce any compile-time breaking changes for anyone who has depended on the `GraphManager` class (or other things you've modified)? I'm wondering if this is a change better suited for the master branch and 3.3.0. If it stays on `tp32` I may have to ask you to provide a second pull request to master that can also be evaluated as part of a second review/vote.
* The addition of `addGraph()` is nice.
* Can you elaborate on the usage of `openGraph()` - I think I get it, but it's not clear to me based on the javadoc/default implementation
* Please add a entry (or more) to CHANGELOG for these updates.
* Please update the reference documentation to include your new Gremlin Server configuration option here: http://tinkerpop.apache.org/docs/current/reference/#_configuring_2 - not sure how much you need to write here. Users of `GraphManager` have likely dug pretty deep into the code and will see how it works. Perhaps include a link to the javadoc for `GraphManager` and beef up the class javadoc there so that implementers know what to do.
Just a bit of warning - from my perspective, this is a fairly involved pull request you've started (and thanks!) so it will likely take a some time and iterations to get through before we can get it merged.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108179063
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,98 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ public Map<String, Graph> getGraphs();
--- End diff --
> Seems like we would want to use GraphManager to enforce the logic of the implementation, thus getGraphs() should go away?
Agreed.
I am confused by your suggestion to "lessen the breaking change", but I'll give a stab at what I think you meant, and we can reiterate
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106415356
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,201 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public Graph getGraph(String gName) {
+ return graphs.get(gName);
+ }
+
+ public void addGraph(String gName, Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public TraversalSource getTraversalSource(String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public void addTraversalSource(String tsName, TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * Basic graphManager has basic openGraph function.
+ */
+ public Graph openGraph(String graphName, Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ return supplier.get();
--- End diff --
Given your explanation of `openGraph()` in your previous comments:
> opengraph() allows for a mechanism by which to instantiate a graph dynamically through the graphManager (and thus allow for reference tracking of it) that is not currently in said graphManager's reference tracking object.
I'd expected to see the result of `supplier.get()` assigned into `graphs` - shouldn't that happen? or no? btw, as i'd mentioned in my earlier comment from a few days ago it would be good to see more javadoc around the `GraphManager` interface methods, as this will now be a public interface that will be promoted as a feature. The explanation you gave made sense - I would just format it to be part of the javadoc.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106424327
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,87 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
-
+ public Map<String, Graph> getGraphs();
+
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
+ */
+ public Graph getGraph(String gName);
+
+ /**
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>} returned by call to getGraphs()
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public void addGraph(String gName, Graph g);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
/**
+ * Get {@link TraversalSource} instance whose name matches {@link tsName}
+ *
+ * @return {@link TraversalSource} if exists, else null
+ */
+
+ public TraversalSource getTraversalSource(String tsName);
+ /**
* Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
*/
- public Bindings getAsBindings() {
- final Bindings bindings = new SimpleBindings();
- graphs.forEach(bindings::put);
- traversalSources.forEach(bindings::put);
- return bindings;
- }
+
+ /**
+ * Add {@link TraversalSource} ts with name {@link String} tsName to
+ * {@link Map<String, TraversalSource>} returned by call to getTraversalSources()
+ */
+ public void addTraversalSource(String tsName, TraversalSource ts);
+
+ public Bindings getAsBindings();
/**
* Rollback transactions across all {@link Graph} objects.
*/
- public void rollbackAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().rollback();
- });
- }
+ public void rollbackAll();
/**
* Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
*/
- public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
- }
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn);
/**
* Commit transactions across all {@link Graph} objects.
*/
- public void commitAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().commit();
- });
- }
+ public void commitAll();
/**
* Selectively commit transactions on the specified graphs or the graphs of traversal sources.
*/
- public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
- }
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn);
/**
- * Selectively close transactions on the specified graphs or the graphs of traversal sources.
+ * Implementation that allows for custom graph-opening implementations.
*/
- private void closeTx(final Set<String> graphSourceNamesToCloseTxOn, final Transaction.Status tx) {
- final Set<Graph> graphsToCloseTxOn = new HashSet<>();
-
- // by the time this method has been called, it should be validated that the source/graph is present.
- // might be possible that it could have been removed dynamically, but that i'm not sure how one would do
- // that as of right now unless they were embedded in which case they'd need to know what they were doing
- // anyway
- graphSourceNamesToCloseTxOn.forEach(r -> {
- if (graphs.containsKey(r))
- graphsToCloseTxOn.add(graphs.get(r));
- else
- graphsToCloseTxOn.add(traversalSources.get(r).getGraph());
- });
+ public Graph openGraph(String graphName, Supplier<Graph> supplier);
- graphsToCloseTxOn.forEach(graph -> {
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen()) {
- if (tx == Transaction.Status.COMMIT)
- graph.tx().commit();
- else
- graph.tx().rollback();
- }
- });
- }
-}
\ No newline at end of file
+ /**
+ * Implementation that allows for custom graph-closing implementations.
+ */
+ public void closeGraph(Graph graph);
--- End diff --
I think I will add both methods and allow the implementor to decide which to use. Personally, I think it makes sense to define a close() method on the object to be closed itself. However, this means the implementor needs to have a `name` parameter associated with their graph, which I don't want to force them to do.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
we can rely on travis as a good basic health check for a PR, but ultimately a full integration test is run on just about everything. at this stage of your PR, i'm mostly interested in the workings of the gremlin-server and gremlin-console integration tests, but making sure your system can do a run of everything isn't a bad idea.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
yeah - that's an obvious truth @pluradj . Normally we wouldn't allow such a change on `tp32` because it does immediately make people's code not compile if they used that class in that way but this class is fairly internal. If someone is using it they likely know what they are doing. @dpitera I tend to agree that this is outside the scope of what would need to remain compatible. That said, I think you will have to add an entry to the upgrade docs (i guess the user section???) about this breaking change and just mention how they go about fixing it (i.e. use the `DefaultGraphManager`). We'll probably also want to send a quick email to the dev list to just alert everyone to a break to make sure no one is buggin' about that. Finally, the JIRA issue will need the "breaking" label added to 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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Feel free to merge this PR @pluradj. The @spmallette bot is currently being rebooted / is on vacation ;).
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Has this been missed by the ASF bot? I see 3 `VOTE: +1`s
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106471272
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Use {@link String} graphName to retrieve {@link Graph} from the {@link Map} tracking
+ * {@link Graph} references, close the {@link Graph}, and remove it from the {@link Map}.
+ */
+ public final void closeGraph(final String graphName) {
+ final Graph graph = getGraph(graphName);
+ graphs.remove(graphName);
+ closeGraph(graph);
+ }
+
+ /**
+ * Close {@link Graph} object.
+ */
+ public final void closeGraph(final Graph graph) {
+ try {
+ graph.close();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
--- End diff --
The more I think about it the less I really understand the use case for `close/openGraph()` when I think about how the `GraphManager` would likely be used. Need to stop thinking about this for today - perhaps i'll think more clearly on it tomorrow and I can formulate a reasonable question or two about 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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108790250
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,101 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Function;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * @Deprecated This returns a {@link Map} that should be immutable. Please refer to
+ * getGraphNames() for replacement.
+ *
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ @Deprecated
+ public Map<String, Graph> getGraphs();
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get a {@link Set} of {@link String} graphNames corresponding to names stored in the graph's
+ * reference tracker.
+ */
+ public Set<String> getGraphNames();
+ /**
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
+ */
+ public Graph getGraph(final String gName);
+
+ /**
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>}
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public void addGraph(final String gName, final Graph g);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
--- End diff --
ha. I haven't quite wrapped my head around the expected use of `traversalSources` so I honestly can speak nothing to this... If you expect that to be the case, I prefer to open a separate issue and make that a separate PR as I can only speak to the graph managing cause right now
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106467302
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Use {@link String} graphName to retrieve {@link Graph} from the {@link Map} tracking
+ * {@link Graph} references, close the {@link Graph}, and remove it from the {@link Map}.
+ */
+ public final void closeGraph(final String graphName) {
+ final Graph graph = getGraph(graphName);
+ graphs.remove(graphName);
+ closeGraph(graph);
+ }
+
+ /**
+ * Close {@link Graph} object.
+ */
+ public final void closeGraph(final Graph graph) {
+ try {
+ graph.close();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
--- End diff --
I'd think it would look like `void closeGraph(final Graph graph) throws Exception`, basically repeating the method on [Graph.close()](https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java#L303).
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
IT tests passed!
```
[INFO] Reactor Summary:
[INFO]
[INFO] Apache TinkerPop .................................. SUCCESS [1:31.220s]
[INFO] Apache TinkerPop :: Gremlin Shaded ................ SUCCESS [12.183s]
[INFO] Apache TinkerPop :: Gremlin Core .................. SUCCESS [1:15.039s]
[INFO] Apache TinkerPop :: Gremlin Test .................. SUCCESS [9.752s]
[INFO] Apache TinkerPop :: Gremlin Groovy ................ SUCCESS [2:50.074s]
[INFO] Apache TinkerPop :: Gremlin Groovy Test ........... SUCCESS [5.952s]
[INFO] Apache TinkerPop :: TinkerGraph Gremlin ........... SUCCESS [4:44.315s]
[INFO] Apache TinkerPop :: Gremlin Benchmark ............. SUCCESS [10.434s]
[INFO] Apache TinkerPop :: Gremlin Driver ................ SUCCESS [15.460s]
[INFO] Apache TinkerPop :: Neo4j Gremlin ................. SUCCESS [4.899s]
[INFO] Apache TinkerPop :: Gremlin Server ................ SUCCESS [16:15.318s]
[INFO] Apache TinkerPop :: Gremlin Python ................ SUCCESS [19.874s]
[INFO] Apache TinkerPop :: Hadoop Gremlin ................ SUCCESS [11:05.683s]
[INFO] Apache TinkerPop :: Spark Gremlin ................. SUCCESS [22:49.909s]
[INFO] Apache TinkerPop :: Giraph Gremlin ................ SUCCESS [2:21:03.271s]
[INFO] Apache TinkerPop :: Gremlin Console ............... SUCCESS [2:56.000s]
[INFO] Apache TinkerPop :: Gremlin Archetype ............. SUCCESS [0.175s]
[INFO] Apache TinkerPop :: Archetype - TinkerGraph ....... SUCCESS [28.255s]
[INFO] Apache TinkerPop :: Archetype - Server ............ SUCCESS [13.480s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 3:26:32.403s
[INFO] Finished at: Thu Mar 30 22:40:41 UTC 2017
[INFO] Final Memory: 103M/771M
[INFO] ------------------------------------------------------------------------
```
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106459384
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Use {@link String} graphName to retrieve {@link Graph} from the {@link Map} tracking
+ * {@link Graph} references, close the {@link Graph}, and remove it from the {@link Map}.
+ */
+ public final void closeGraph(final String graphName) {
+ final Graph graph = getGraph(graphName);
+ graphs.remove(graphName);
+ closeGraph(graph);
+ }
+
+ /**
+ * Close {@link Graph} object.
+ */
+ public final void closeGraph(final Graph graph) {
+ try {
+ graph.close();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
--- End diff --
I agree-- this implementation of a graphManager makes sense for users t call `closeGraph(String)`. Suggestions on what to do in the catch block?
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106248061
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/BasicGraphManager.java ---
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class BasicGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public BasicGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public Graph getGraph(String gName) {
+ return graphs.get(gName);
+ }
+
+ public void addGraph(String gName, Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public TraversalSource getTraversalSource(String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public void addTraversalSource(String tsName, TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * Basic graphManager has basic openGraph function.
--- End diff --
javadoc should mention that the `Supplier` is not actually used in this default implementation class.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/569
No, it is mentioned here http://tinkerpop.apache.org/docs/3.2.4/dev/developer/#_contributing_code_changes
Devs often use Docker for to run it `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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106410060
--- Diff: CHANGELOG.asciidoc ---
@@ -47,6 +47,9 @@ TinkerPop 3.2.5 (Release Date: NOT OFFICIALLY RELEASED YET)
* Improved error handling of compilation failures for very large or highly parameterized script sent to Gremlin Server.
* Fixed a bug in `RangeByIsCountStrategy` that changed the meaning of inner traversals.
* Improved Gremlin-Python Driver implementation by adding a threaded client with basic connection pooling and support for pluggable websocket clients.
+* Changed GraphManager from a final class implementation to an interface.
--- End diff --
minor formatting thing - make sure that class/interface/code names have the backtick around them so that they format properly when the asciidoc is generated. you might want to look at the other docs you wrote as you missed a few instances there as well.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106410530
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,87 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
-
+ public Map<String, Graph> getGraphs();
+
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
+ */
+ public Graph getGraph(String gName);
--- End diff --
Looks like you're still missing a bunch of "final" declarations. I would normally just handle them for you at merge, but you have a pretty large PR going here, so it would be helpful if you got them all for us.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106462086
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Use {@link String} graphName to retrieve {@link Graph} from the {@link Map} tracking
+ * {@link Graph} references, close the {@link Graph}, and remove it from the {@link Map}.
+ */
+ public final void closeGraph(final String graphName) {
+ final Graph graph = getGraph(graphName);
+ graphs.remove(graphName);
+ closeGraph(graph);
+ }
+
+ /**
+ * Close {@link Graph} object.
+ */
+ public final void closeGraph(final Graph graph) {
+ try {
+ graph.close();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
--- End diff --
And make the `DefaultGraphManager` not call `graph.close()`?
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106412544
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,201 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public Graph getGraph(String gName) {
+ return graphs.get(gName);
+ }
+
+ public void addGraph(String gName, Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public TraversalSource getTraversalSource(String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public void addTraversalSource(String tsName, TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * Basic graphManager has basic openGraph function.
+ */
+ public Graph openGraph(String graphName, Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ return supplier.get();
+ }
+
+ /**
+ * Close graph
+ */
+ public void closeGraph(Graph graph) {
+ try {
+ graph.close();
--- End diff --
What does it mean to actually call `closeGraph()`? If "closed" does that mean it should still be in the available set of graphs? If so it would still be accessible to requests but it would be in a closed state. Different graphs will have different semantics with respect to what `close()` will do. Anyway, just posing the issue for thought at this point....not sure i have a strong opinion yet on what the behavior should be.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
GitHub user dpitera reopened a pull request:
https://github.com/apache/tinkerpop/pull/569
TINKERPOP-1438: GraphManager becomes a customizable interface
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dpitera/tinkerpop TINKERPOP-1438
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/569.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 #569
----
commit 1f2cfa04bfb5c6758aecb2ff69a4a7942c1701f0
Author: Benjamin Anderson <b...@banjiewen.net>
Date: 2016-08-20T05:33:16Z
Replace GraphManager with interface
This enabled settings-based customization of the GraphManager
implementation class, allowing implementors to customize the behavior of
the GraphManager.
commit 67471d13b5fa05d19d51f117a95530a81011f792
Author: dpitera <dp...@us.ibm.com>
Date: 2016-11-21T18:01:47Z
GraphManager support opening of specific graphs
This changeset allows an implementor to open a specific graph. One may
use this concept to implement a dynamic graph cache.
Furthermore, to ensure that rebindings work as intended, i.e. the list
of graphs returned to the HttpGremlinEndpointHandler, or "open graphs",
must include the to-be-rebound-graph. This changeset includes a change
to return these rebound graphs specifically.
Similar story as above for the WebSockets class, StandardOpProcessor.
Similar story for sessions, SessionOpProcessor.
Furthermore, the serializers at server boot only need to be aware of the
graphs defined in the settings object, so the relevant change here is in
AbstractChannelizer.
Furthermore:
To encourage a GraphManager implementation whose modification of the
Map<String, Graph> object aligns more closely with accepted "Getter and
Setter" design patterns, we update the adding of graphs to the
GraphManager Map by calling the new `addGraph(String, Graph)` rather
than publicly editting it with a call to `getGraphs().put(String,
Graph)`.
Also added `addTraversalSource(String, TraversalSource) for same
reasons.
Also, updated BasicGraphManager according to the new specifications.
commit 4f9f90b995aacba344d0b4c54ef2600f66fcbb48
Author: dpitera <dp...@us.ibm.com>
Date: 2017-03-15T19:31:45Z
Allow for custom graph instantiations/closings
This allows an implementor to supply a custom openGraph function to
return a newly instantiated graph object, and similarly do the same to
close a graph object, while doing so through the graphManager for
reference tracking.
commit 32374d9442229930b2713a35ec4c44aadff6c732
Author: dpitera <dp...@us.ibm.com>
Date: 2017-03-15T19:34:09Z
Update docs acc. to GraphManager changes
commit 8aa66e19037ace4443d19c90591b5a86fea09cfa
Author: dpitera <dp...@us.ibm.com>
Date: 2017-03-16T15:32:47Z
Update code according to PR comments/suggestions
----
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
@spmallette No rush, just checking in. Have you thought about this anymore? I had addressed some of your questions [here](https://github.com/apache/tinkerpop/pull/569#discussion_r106475154) and I have pushed code addressing feedback.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
> have you run the full integration test suite to success on this?
Was that not run by the CI build [here](https://travis-ci.org/apache/tinkerpop/builds/208345874)
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/569
It would break if somebody was calling on `new GraphManager(Settings)`. You could change the interface to `IGraphManager` and have `GraphManager` implement 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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/569
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108175942
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,98 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ public Map<String, Graph> getGraphs();
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public Graph getGraph(final String gName);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>} returned by call to getGraphs()
+ */
+ public void addGraph(final String gName, final Graph g);
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
+
+ /**
+ * Get {@link TraversalSource} instance whose name matches {@link tsName}
+ *
+ * @return {@link TraversalSource} if exists, else null
+ */
+ public TraversalSource getTraversalSource(final String tsName);
/**
* Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
*/
- public Bindings getAsBindings() {
- final Bindings bindings = new SimpleBindings();
- graphs.forEach(bindings::put);
- traversalSources.forEach(bindings::put);
- return bindings;
- }
+
+ /**
+ * Add {@link TraversalSource} ts with name {@link String} tsName to
+ * {@link Map<String, TraversalSource>} returned by call to getTraversalSources()
+ */
+ public void addTraversalSource(final String tsName, final TraversalSource ts);
+
+ public Bindings getAsBindings();
/**
* Rollback transactions across all {@link Graph} objects.
*/
- public void rollbackAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().rollback();
- });
- }
+ public void rollbackAll();
/**
* Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
*/
- public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
- }
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn);
/**
* Commit transactions across all {@link Graph} objects.
*/
- public void commitAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().commit();
- });
- }
+ public void commitAll();
/**
* Selectively commit transactions on the specified graphs or the graphs of traversal sources.
*/
- public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
- }
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn);
+
+ /**
+ * Implementation that allows for custom graph-opening implementations; if the {@link Map}
+ * tracking graph references has a {@link Graph} object corresponding to the {@link String} graphName,
+ * then we return that {@link Graph}-- otherwise, we use the custom {@link Supplier} to instantiate a
+ * a new {@link Graph}, add it to the {@link Map} tracking graph references, and return said {@link Graph}.
+ */
+ public Graph openGraph(final String graphName, final Supplier<Graph> supplier);
--- End diff --
Sounds good to me.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r109934781
--- Diff: docs/src/reference/gremlin-applications.asciidoc ---
@@ -1075,6 +1075,7 @@ The following table describes the various configuration options that Gremlin Ser
|authentication.className |The fully qualified classname of an `Authenticator` implementation to use. If this setting is not present, then authentication is effectively disabled. |`AllowAllAuthenticator`
|authentication.config |A `Map` of configuration settings to be passes to the `Authenticator` when it is constructed. The settings available are dependent on the implementation. |_none_
|channelizer |The fully qualified classname of the `Channelizer` implementation to use. A `Channelizer` is a "channel initializer" which Gremlin Server uses to define the type of processing pipeline to use. By allowing different `Channelizer` implementations, Gremlin Server can support different communication protocols (e.g. Websockets, Java NIO, etc.). |`WebSocketChannelizer`
+|graphManager |The fully qualified classname of the `GraphManager` implementation to use. A `GraphManager` is a class that adheres to the Tinkerpop `GraphManager` interface, allowing custom implementations for storing and managing graph references, as well as defining custom methods to open and close graphs instantiations. It is important to note that the Tinkerpop Http and WebSocketChannelizers auto-commit and auto-rollback based on the graphs stored in the graphManager upon script execution completion. |`DefaultGraphManager`
--- End diff --
TinkerPop -- can fix case on commit
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
I've finished playing around with my dummy implementation. Just one weird thing that wasn't easy to see without actually working with the API - `addTraversalSource()` and `addGraph()` aren't really "adds" - they are "puts" as calling them more than once will act as a "replace" operation if the key already exists. I think we want the behavior to generally be a "replace" and thus "putTraversalSource()" and "putGraph()" seem like better names. I suppose if an implementation wanted to limit that ability to only "adds" then the naming with "put" still makes sense as we can see with the use of `Map.put()` in unmodifiable collections.
I see some other odds/ends that could use a tweak or two, but I'll just handle those things after merging as I think you have the meat of this PR in place now once you address those other comments I've made. Please confirm that you've done a final build with integration tests after all these changes to make sure it's all solid. At that point, I expect we can get committers to review/vote.
This has developed into a really nice pull request. Your effort on it is appreciated.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106217616
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,82 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
-
+ public Map<String, Graph> getGraphs();
+
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public Graph getGraph(String gName);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>} returned by call to getGraphs()
+ */
+ public void addGraph(String gName, Graph g);
--- End diff --
seems like `removeGraph(String)` or `removeGraph(Graph)` would be useful
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Thanks Jason! Reading/running now
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108759155
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,101 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Function;
+public interface GraphManager {
--- End diff --
Since we don't really have any dev docs for how to implement a `GraphManager` could you please add some javadoc here to describe what this interface is for and why someone would implement 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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r107677257
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,98 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ public Map<String, Graph> getGraphs();
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public Graph getGraph(final String gName);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>} returned by call to getGraphs()
+ */
+ public void addGraph(final String gName, final Graph g);
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
+
+ /**
+ * Get {@link TraversalSource} instance whose name matches {@link tsName}
+ *
+ * @return {@link TraversalSource} if exists, else null
+ */
+ public TraversalSource getTraversalSource(final String tsName);
/**
* Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
*/
- public Bindings getAsBindings() {
- final Bindings bindings = new SimpleBindings();
- graphs.forEach(bindings::put);
- traversalSources.forEach(bindings::put);
- return bindings;
- }
+
+ /**
+ * Add {@link TraversalSource} ts with name {@link String} tsName to
+ * {@link Map<String, TraversalSource>} returned by call to getTraversalSources()
+ */
+ public void addTraversalSource(final String tsName, final TraversalSource ts);
+
+ public Bindings getAsBindings();
/**
* Rollback transactions across all {@link Graph} objects.
*/
- public void rollbackAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().rollback();
- });
- }
+ public void rollbackAll();
/**
* Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
*/
- public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
- }
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn);
/**
* Commit transactions across all {@link Graph} objects.
*/
- public void commitAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().commit();
- });
- }
+ public void commitAll();
/**
* Selectively commit transactions on the specified graphs or the graphs of traversal sources.
*/
- public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
- }
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn);
+
+ /**
+ * Implementation that allows for custom graph-opening implementations; if the {@link Map}
+ * tracking graph references has a {@link Graph} object corresponding to the {@link String} graphName,
+ * then we return that {@link Graph}-- otherwise, we use the custom {@link Supplier} to instantiate a
+ * a new {@link Graph}, add it to the {@link Map} tracking graph references, and return said {@link Graph}.
+ */
+ public Graph openGraph(final String graphName, final Supplier<Graph> supplier);
+
+ /**
+ * Implementation that allows for custom graph-closing implementations; it is up to the implementor
+ * to decide if this method should also remove the {@link Graph} graph from the {@link Map}
+ * tracking {@link Graph} references (and how it would do so).
+ */
+ public void closeGraph(final Graph graph) throws Exception;
--- End diff --
I've made a number of comments about `closeGraph()` so another one shouldn't be surprising :smile: - I can sorta understand the requirement here as the symmetrical method to `openGraph()` and the idea that the component interacting with the manager won't keep track of the graph names and may only have the `Graph` instance. So, I'm that far along....
So these would be the current thoughts/concerns:
1. The javadoc makes this implementation have a pretty wide open scope leaving it up to the implementation to choose whether or not the `Graph` is removed from the "registry" (i.e. control of the `GraphManager`) - you mention `Map` specifically but the registry might not use that data structure specifically I suppose. Under what circumstance would an interacting component call `closeGraph()` but not want the `Graph` removed from the registry? To leave it there would allow callers to Gremlin Server to get graphs that were in a "closed" state which doesn't seem right.
2. Piggy-backing on that last point, if you go down to `DefaultGraphManager`, I think that it should look to remove the instance from the registry. Again, I'm lost for a scenario where you would want a closed `Graph` instance available to Gremlin Server requests.
3. I still think this method is really just an overload of `removeGraph(String)` and should be named as such: `Graph removeGraph(Graph)`. A bit weird as you already have the `Graph` instance and are just returning it back, but it would let the API be more fluent. I think @pluradj implied the same thing in a different comment.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Ran Gremlin Server integration tests locally to success - 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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Well done.
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106424001
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,201 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public Graph getGraph(String gName) {
+ return graphs.get(gName);
+ }
+
+ public void addGraph(String gName, Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public TraversalSource getTraversalSource(String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public void addTraversalSource(String tsName, TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * Basic graphManager has basic openGraph function.
+ */
+ public Graph openGraph(String graphName, Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ return supplier.get();
--- End diff --
> I'd expected to see the result of supplier.get() assigned into graphs
Asbolutely
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108180988
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,98 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ public Map<String, Graph> getGraphs();
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public Graph getGraph(final String gName);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>} returned by call to getGraphs()
+ */
+ public void addGraph(final String gName, final Graph g);
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
+
+ /**
+ * Get {@link TraversalSource} instance whose name matches {@link tsName}
+ *
+ * @return {@link TraversalSource} if exists, else null
+ */
+ public TraversalSource getTraversalSource(final String tsName);
/**
* Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
*/
- public Bindings getAsBindings() {
- final Bindings bindings = new SimpleBindings();
- graphs.forEach(bindings::put);
- traversalSources.forEach(bindings::put);
- return bindings;
- }
+
+ /**
+ * Add {@link TraversalSource} ts with name {@link String} tsName to
+ * {@link Map<String, TraversalSource>} returned by call to getTraversalSources()
+ */
+ public void addTraversalSource(final String tsName, final TraversalSource ts);
+
+ public Bindings getAsBindings();
/**
* Rollback transactions across all {@link Graph} objects.
*/
- public void rollbackAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().rollback();
- });
- }
+ public void rollbackAll();
/**
* Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
*/
- public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
- }
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn);
/**
* Commit transactions across all {@link Graph} objects.
*/
- public void commitAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().commit();
- });
- }
+ public void commitAll();
/**
* Selectively commit transactions on the specified graphs or the graphs of traversal sources.
*/
- public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
- }
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn);
+
+ /**
+ * Implementation that allows for custom graph-opening implementations; if the {@link Map}
+ * tracking graph references has a {@link Graph} object corresponding to the {@link String} graphName,
+ * then we return that {@link Graph}-- otherwise, we use the custom {@link Supplier} to instantiate a
+ * a new {@link Graph}, add it to the {@link Map} tracking graph references, and return said {@link Graph}.
+ */
+ public Graph openGraph(final String graphName, final Supplier<Graph> supplier);
+
+ /**
+ * Implementation that allows for custom graph-closing implementations; it is up to the implementor
+ * to decide if this method should also remove the {@link Graph} graph from the {@link Map}
+ * tracking {@link Graph} references (and how it would do so).
+ */
+ public void closeGraph(final Graph graph) throws Exception;
--- End diff --
> I agree, but if we do not have the String representation of the graphName here then how can we do that?
I guess you'd have to iterate the `Graph` values in `GraphManager` and compare for equality. I suppose you would be passing the same instance in to `closeGraph()` as is held there?
Sorry I'm struggling to understand the need for this method. Perhaps you will need to link me to your wip for JanusGraph so that I can better see where you are coming from.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106413754
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,87 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
-
+ public Map<String, Graph> getGraphs();
+
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
+ */
+ public Graph getGraph(String gName);
+
+ /**
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>} returned by call to getGraphs()
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public void addGraph(String gName, Graph g);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
/**
+ * Get {@link TraversalSource} instance whose name matches {@link tsName}
+ *
+ * @return {@link TraversalSource} if exists, else null
+ */
+
+ public TraversalSource getTraversalSource(String tsName);
+ /**
* Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
*/
- public Bindings getAsBindings() {
- final Bindings bindings = new SimpleBindings();
- graphs.forEach(bindings::put);
- traversalSources.forEach(bindings::put);
- return bindings;
- }
+
+ /**
+ * Add {@link TraversalSource} ts with name {@link String} tsName to
+ * {@link Map<String, TraversalSource>} returned by call to getTraversalSources()
+ */
+ public void addTraversalSource(String tsName, TraversalSource ts);
+
+ public Bindings getAsBindings();
/**
* Rollback transactions across all {@link Graph} objects.
*/
- public void rollbackAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().rollback();
- });
- }
+ public void rollbackAll();
/**
* Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
*/
- public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
- }
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn);
/**
* Commit transactions across all {@link Graph} objects.
*/
- public void commitAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().commit();
- });
- }
+ public void commitAll();
/**
* Selectively commit transactions on the specified graphs or the graphs of traversal sources.
*/
- public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
- }
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn);
/**
- * Selectively close transactions on the specified graphs or the graphs of traversal sources.
+ * Implementation that allows for custom graph-opening implementations.
*/
- private void closeTx(final Set<String> graphSourceNamesToCloseTxOn, final Transaction.Status tx) {
- final Set<Graph> graphsToCloseTxOn = new HashSet<>();
-
- // by the time this method has been called, it should be validated that the source/graph is present.
- // might be possible that it could have been removed dynamically, but that i'm not sure how one would do
- // that as of right now unless they were embedded in which case they'd need to know what they were doing
- // anyway
- graphSourceNamesToCloseTxOn.forEach(r -> {
- if (graphs.containsKey(r))
- graphsToCloseTxOn.add(graphs.get(r));
- else
- graphsToCloseTxOn.add(traversalSources.get(r).getGraph());
- });
+ public Graph openGraph(String graphName, Supplier<Graph> supplier);
- graphsToCloseTxOn.forEach(graph -> {
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen()) {
- if (tx == Transaction.Status.COMMIT)
- graph.tx().commit();
- else
- graph.tx().rollback();
- }
- });
- }
-}
\ No newline at end of file
+ /**
+ * Implementation that allows for custom graph-closing implementations.
+ */
+ public void closeGraph(Graph graph);
--- End diff --
I wonder if the signature for this method should be:
```java
public void closeGraph(final String graphName);
```
maybe i'm missing the point of the method...i'm just basing this on my interpretation of the implementation in `DefaultGraphManager`.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
I've been really focused on some other things the last few days. Expected to focus on pull requests today (which are stacking up). Of course, the one I started with has me all hung up. Hoping to make my rounds to all of them today, including this one.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106454863
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Use {@link String} graphName to retrieve {@link Graph} from the {@link Map} tracking
+ * {@link Graph} references, close the {@link Graph}, and remove it from the {@link Map}.
+ */
+ public final void closeGraph(final String graphName) {
--- End diff --
@spmallette I had suggested `removeGraph(String)` for this as the reciprocal for `addGraph(String, Graph)`. I wouldn't expect remove graph to close the graph since add graph didn't open 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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
> I think we want the behavior to generally be a "replace" and thus "putTraversalSource()" and "putGraph()" seem like better names.
+1
> This has developed into a really nice pull request. Your effort on it is appreciated.
Thank you sir. Your outstanding review is also very much appreciated.
I just pushed new code with all requested changes; once the TravisCI and Docker IT tests pass, I will post again for hopefully the final review.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
thanks for submitting this. i didn't realize you'd get something out so quickly. before i dig too deeply into this have you run the full integration test suite to success on this?
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
> It would break if somebody was calling on new GraphManager(Settings)
If someone were doing this, it would mean they have had to reconstruct a `Settings` object, which would be different than that instantiated inside the `GremlinServer` and I would argue this is _outside the scope_ of something we need to be compatible with, i.e. using a modified version of Tinkerpop, or using it in an unexpected way. What are your thoughts here?
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
I'll work on this one today as soon assuming I get caught up on all missed emails and such. I sense the merge to master won't be trivial, but we'll see. Anyway, this one should close out soon.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r106475154
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java ---
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
+ * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
+ * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
+ * initialization scripts construct them.
+ */
+public final class DefaultGraphManager implements GraphManager {
+ private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
+
+ private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
+ private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+
+ /**
+ * Create a new instance using the {@link Settings} from Gremlin Server.
+ */
+ public DefaultGraphManager(final Settings settings) {
+ settings.graphs.entrySet().forEach(e -> {
+ try {
+ final Graph newGraph = GraphFactory.open(e.getValue());
+ graphs.put(e.getKey(), newGraph);
+ logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
+ } catch (RuntimeException re) {
+ logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
+ e.getKey(), e.getValue(), re.getMessage()), re);
+ if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
+ }
+ });
+ }
+
+ /**
+ * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
+ * configuration file.
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ */
+ public final Map<String, Graph> getGraphs() {
+ return graphs;
+ }
+
+ public final Graph getGraph(final String gName) {
+ return graphs.get(gName);
+ }
+
+ public final void addGraph(final String gName, final Graph g) {
+ graphs.put(gName, g);
+ }
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
+ * initialization scripts.
+ *
+ * @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
+ * {@link TraversalSource} itself
+ */
+ public final Map<String, TraversalSource> getTraversalSources() {
+ return traversalSources;
+ }
+
+ public final TraversalSource getTraversalSource(final String tsName) {
+ return traversalSources.get(tsName);
+ }
+
+ public final void addTraversalSource(final String tsName, final TraversalSource ts) {
+ traversalSources.put(tsName, ts);
+ }
+
+ /**
+ * Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
+ */
+ public final Bindings getAsBindings() {
+ final Bindings bindings = new SimpleBindings();
+ graphs.forEach(bindings::put);
+ traversalSources.forEach(bindings::put);
+ return bindings;
+ }
+
+ /**
+ * Rollback transactions across all {@link Graph} objects.
+ */
+ public final void rollbackAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().rollback();
+ });
+ }
+
+ /**
+ * Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
+ }
+
+ /**
+ * Commit transactions across all {@link Graph} objects.
+ */
+ public final void commitAll() {
+ graphs.entrySet().forEach(e -> {
+ final Graph graph = e.getValue();
+ if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
+ graph.tx().commit();
+ });
+ }
+
+ /**
+ * Selectively commit transactions on the specified graphs or the graphs of traversal sources.
+ */
+ public final void commit(final Set<String> graphSourceNamesToCloseTxOn) {
+ closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
+ }
+
+ /**
+ * If {@link Map} containing {@link Graph} references contains one corresponding to
+ * {@link String} graphName, then we return that {@link Graph}; otherwise, instantiate a
+ * new {@link Graph} using the {@link Supplier}, add it to the {@link Map} tracking {@link Graph}
+ * references, and return that {@link Graph}.
+ */
+ public final Graph openGraph(final String graphName, final Supplier<Graph> supplier) {
+ final Graph graph = graphs.get(graphName);
+ if (null != graph) {
+ return graph;
+ }
+ final Graph newGraph = supplier.get();
+ addGraph(graphName, newGraph);
+ return newGraph;
+ }
+
+ /**
+ * Use {@link String} graphName to retrieve {@link Graph} from the {@link Map} tracking
+ * {@link Graph} references, close the {@link Graph}, and remove it from the {@link Map}.
+ */
+ public final void closeGraph(final String graphName) {
+ final Graph graph = getGraph(graphName);
+ graphs.remove(graphName);
+ closeGraph(graph);
+ }
+
+ /**
+ * Close {@link Graph} object.
+ */
+ public final void closeGraph(final Graph graph) {
+ try {
+ graph.close();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
--- End diff --
If it will help make more sense to you, I can show you my WIP implementation for JanusGraph. I hadn't brought it up to avoid clouding decisions made here based on the needs of a specific implementation.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108760283
--- Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManagerTest.java ---
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.util;
+
+import org.apache.tinkerpop.gremlin.server.GraphManager;
+import org.apache.tinkerpop.gremlin.server.Settings;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerGraph;
+import org.junit.Test;
+
+import javax.script.Bindings;
+import java.util.Map;
+import java.util.Set;
+
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+
+/**
+ * @author Stephen Mallette (http://stephen.genoprime.com)
+ */
+public class DefaultGraphManagerTest {
--- End diff --
Would you mind adding appropriate tests for the code in `openGraph()`? Not sure if there's other methods/code branches that require testing or not, but perhaps give that a review now that we've better settled the API.
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera closed the pull request at:
https://github.com/apache/tinkerpop/pull/569
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
@spmallette Updated with your suggestions; only thing i have not addressed is `closeGraph()` versus `removeGraph()`.
---
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] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Using the GitHub review seems to have made my vote disappear...
Successful `docker/build.sh -t -i -n`. Verified docs and javadocs. Solid work on this @dpitera.
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r108197676
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java ---
@@ -21,139 +21,98 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger = LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via [{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s] could not be instantiated and will not be available in Gremlin Server. GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory exception", re.getCause());
- }
- });
- }
+ public Map<String, Graph> getGraphs();
/**
- * Get a list of the {@link Graph} instances and their binding names as defined in the Gremlin Server
- * configuration file.
+ * Get {@link Graph} instance whose name matches {@link gName}
*
- * @return a {@link Map} where the key is the name of the {@link Graph} and the value is the {@link Graph} itself
+ * @return {@link Graph} if exists, else null
*/
- public Map<String, Graph> getGraphs() {
- return graphs;
- }
+ public Graph getGraph(final String gName);
/**
- * Get a list of the {@link TraversalSource} instances and their binding names as defined by Gremlin Server
- * initialization scripts.
+ * Add {@link Graph} g with name {@link String} gName to
+ * {@link Map<String, Graph>} returned by call to getGraphs()
+ */
+ public void addGraph(final String gName, final Graph g);
+
+ /**
+ * Get a list of the {@link TraversalSource} instances and their binding names
*
* @return a {@link Map} where the key is the name of the {@link TraversalSource} and the value is the
* {@link TraversalSource} itself
*/
- public Map<String, TraversalSource> getTraversalSources() {
- return traversalSources;
- }
+ public Map<String, TraversalSource> getTraversalSources();
+
+ /**
+ * Get {@link TraversalSource} instance whose name matches {@link tsName}
+ *
+ * @return {@link TraversalSource} if exists, else null
+ */
+ public TraversalSource getTraversalSource(final String tsName);
/**
* Get the {@link Graph} and {@link TraversalSource} list as a set of bindings.
*/
- public Bindings getAsBindings() {
- final Bindings bindings = new SimpleBindings();
- graphs.forEach(bindings::put);
- traversalSources.forEach(bindings::put);
- return bindings;
- }
+
+ /**
+ * Add {@link TraversalSource} ts with name {@link String} tsName to
+ * {@link Map<String, TraversalSource>} returned by call to getTraversalSources()
+ */
+ public void addTraversalSource(final String tsName, final TraversalSource ts);
+
+ public Bindings getAsBindings();
/**
* Rollback transactions across all {@link Graph} objects.
*/
- public void rollbackAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().rollback();
- });
- }
+ public void rollbackAll();
/**
* Selectively rollback transactions on the specified graphs or the graphs of traversal sources.
*/
- public void rollback(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.ROLLBACK);
- }
+ public void rollback(final Set<String> graphSourceNamesToCloseTxOn);
/**
* Commit transactions across all {@link Graph} objects.
*/
- public void commitAll() {
- graphs.entrySet().forEach(e -> {
- final Graph graph = e.getValue();
- if (graph.features().graph().supportsTransactions() && graph.tx().isOpen())
- graph.tx().commit();
- });
- }
+ public void commitAll();
/**
* Selectively commit transactions on the specified graphs or the graphs of traversal sources.
*/
- public void commit(final Set<String> graphSourceNamesToCloseTxOn) {
- closeTx(graphSourceNamesToCloseTxOn, Transaction.Status.COMMIT);
- }
+ public void commit(final Set<String> graphSourceNamesToCloseTxOn);
+
+ /**
+ * Implementation that allows for custom graph-opening implementations; if the {@link Map}
+ * tracking graph references has a {@link Graph} object corresponding to the {@link String} graphName,
+ * then we return that {@link Graph}-- otherwise, we use the custom {@link Supplier} to instantiate a
+ * a new {@link Graph}, add it to the {@link Map} tracking graph references, and return said {@link Graph}.
+ */
+ public Graph openGraph(final String graphName, final Supplier<Graph> supplier);
+
+ /**
+ * Implementation that allows for custom graph-closing implementations; it is up to the implementor
+ * to decide if this method should also remove the {@link Graph} graph from the {@link Map}
+ * tracking {@link Graph} references (and how it would do so).
+ */
+ public void closeGraph(final Graph graph) throws Exception;
--- End diff --
This is my current WIP implementation of [closeGraph](https://github.com/dpitera/janusgraph/blob/testing/janusgraph-core/src/main/java/org/janusgraph/graphdb/management/JanusGraphManager.java#L190) and [openGraph](https://github.com/dpitera/janusgraph/blob/testing/janusgraph-core/src/main/java/org/janusgraph/graphdb/management/JanusGraphManager.java#L145)
---
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] tinkerpop pull request #569: TINKERPOP-1438: GraphManager becomes a customiz...
Posted by dpitera <gi...@git.apache.org>.
Github user dpitera closed the pull request at:
https://github.com/apache/tinkerpop/pull/569
---
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.
---