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 [email protected] or file a JIRA ticket
with INFRA.
---
