[
https://issues.apache.org/jira/browse/TINKERPOP-3290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18138474#comment-18138474
]
ASF GitHub Bot commented on TINKERPOP-3290:
-------------------------------------------
kenhuuu commented on code in PR #3760:
URL: https://github.com/apache/tinkerpop/pull/3760#discussion_r4476296061
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/DefaultGraphManager.java:
##########
@@ -173,26 +171,48 @@ public final Graph removeGraph(final String graphName)
throws Exception {
* Selectively close transactions on the specified graphs or the graphs of
traversal sources.
*/
private void closeTx(final Set<String> graphSourceNamesToCloseTxOn, final
Transaction.Status tx) {
- final Set<Graph> graphsToCloseTxOn = new HashSet<>();
+ // a graph and a traversal source over it may both be named, so key by
graph to close its transaction once
+ final Map<Graph, String> graphsToCloseTxOn = new HashMap<>();
// 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));
+ graphsToCloseTxOn.putIfAbsent(graphs.get(r), r);
else
- graphsToCloseTxOn.add(traversalSources.get(r).getGraph());
+
graphsToCloseTxOn.putIfAbsent(traversalSources.get(r).getGraph(), r);
});
- graphsToCloseTxOn.forEach(graph -> {
- if (graph.features().graph().supportsTransactions() &&
graph.tx().isOpen()) {
+ graphsToCloseTxOn.forEach((graph, name) -> {
+ final Transaction openTx = openTransaction(name, graph);
+ if (openTx != null) {
if (tx == Transaction.Status.COMMIT)
- graph.tx().commit();
+ openTx.commit();
else
- graph.tx().rollback();
+ openTx.rollback();
}
});
}
+
+ /**
+ * Gets the open transaction of a graph, or {@code null} if the graph does
not support transactions, has none open,
+ * or is closed. A closed graph is logged and skipped so that it does not
fail the sweep for the other graphs, and it
+ * stays registered.
+ */
+ private static Transaction openTransaction(final String name, final Graph
graph) {
Review Comment:
This doesn't take into account how `UnmanagedTransaction` works.
Transactions are threaded and so this only really checks if a Transaction is
open on the current thread (which is almost certainly none), not if any
transactions are open. See this related jira
https://issues.apache.org/jira/browse/TINKERPOP-3226
It's probably fine to just remove all the changes in this file and just make
a Jira for it. Transaction lifecycle in the server needs some cleaning up and
there are already open Jiras for that so it shouldn't block this PR.
> TinkerGraph close() semantics should prevent further usage
> ----------------------------------------------------------
>
> Key: TINKERPOP-3290
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3290
> Project: TinkerPop
> Issue Type: Improvement
> Components: tinkergraph
> Affects Versions: 3.8.2
> Reporter: Stephen Mallette
> Priority: Critical
>
> {code}
> gremlin> graph = TinkerGraph.open()
> ==>tinkergraph[vertices:0 edges:0]
> gremlin> g = traversal().with(graph)
> ==>graphtraversalsource[tinkergraph[vertices:0 edges:0], standard]
> gremlin> g.addV('person')
> ==>v[0]
> gremlin> g.V().count()
> ==>1
> gremlin> graph.close()
> ==>null
> gremlin> g.addV('person')
> ==>v[1]
> gremlin> g.V().count()
> ==>2
> {code}
> This will be at odds with the file-persistent version of TinkerGraph in 4.0
> where we will need {{close()} to actually prevent further usage. Need to
> align the pure in-memory graph with that.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
