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