[
https://issues.apache.org/jira/browse/TINKERPOP-3282?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18120426#comment-18120426
]
ASF GitHub Bot commented on TINKERPOP-3282:
-------------------------------------------
Cole-Greer commented on code in PR #3639:
URL: https://github.com/apache/tinkerpop/pull/3639#discussion_r4127651155
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerGraph.java:
##########
@@ -337,13 +322,31 @@ public void clear() {
}
/**
- * This method only has an effect if the {@link
TinkerGraph#GREMLIN_TINKERGRAPH_GRAPH_LOCATION} is set, in which case the
- * data in the graph is persisted to that location. This method may be
called multiple times and does not release
- * resources.
+ * Closes the graph, releasing any resources held by its {@link
TinkerServiceRegistry}. This method may be called
+ * multiple times and is a no-op with respect to graph data for the
in-memory implementation. Transactional
+ * implementations that are backed by a {@link
org.apache.tinkerpop.gremlin.tinkergraph.structure.storage.TinkerStorage}
+ * engine flush and close that engine here.
*/
@Override
public void close() {
- if (graphLocation != null) saveGraph();
+ if (storage != null) {
Review Comment:
Nit: I wonder if we're better off pushing this down to the concrete classes.
It feels a bit weird having logic concerning only `TinkerStorageGraph` in the
shared abstract class.
##########
gremlin-server/conf/tinkerstoragegraph-persistent.properties:
##########
@@ -0,0 +1,40 @@
+# 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.
+
+# Sample configuration for a durable, transactional TinkerStorageGraph on
Gremlin Server. Reference it from the
+# server's "graphs" block, for example in gremlin-server-transaction.yaml:
Review Comment:
I think the extra reference creates a maintenance chore without adding much
value. Also it's referencing how `gremlin-server-transaction.yaml` references
this file, yet the server config yamls are unchanged in this PR.
```suggestion
# Sample configuration for a durable, transactional TinkerStorageGraph on
Gremlin Server.
```
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/auth/SimpleAuthenticator.java:
##########
@@ -80,12 +83,63 @@ public void setup(final Map<String,Object> config) {
// have to create the indices because they are not stored in gryo
final TinkerGraph tinkerGraph = (TinkerGraph) graph;
tinkerGraph.createIndex(PROPERTY_USERNAME, Vertex.class);
+
+ // TinkerGraph no longer auto-loads from graphLocation on open, so
read the credential store here from
+ // the configured location/format. TinkerStorageGraph (which
persists via its own storage engine) manages
+ // its own data and is left untouched.
+ loadCredentialStore(tinkerGraph);
}
credentialStore = graph.traversal(CredentialTraversalSource.class);
logger.info("CredentialGraph initialized at {}", credentialStore);
}
+ /**
+ * Reads the credential store into the supplied in-memory {@link
TinkerGraph} from the
+ * {@code gremlin.tinkergraph.graphLocation} and {@code
gremlin.tinkergraph.graphFormat} entries of its
+ * configuration, if present. Earlier versions of TinkerGraph read those
keys themselves on open, so the
+ * credential store loaded as a side effect of {@code GraphFactory.open};
that automatic behaviour was removed
+ * and this method preserves it for the credential store alone.
+ * <p/>
+ * No TinkerGraph reads either key any more, which is why they are named
here as literals rather than through
+ * constants. Despite the {@code gremlin.tinkergraph} prefix they are in
effect settings of this authenticator,
+ * and belong in its own {@code config} block beside {@code credentialsDb}
rather than in the graph's properties
+ * file. They are left in place here only to keep existing credential
configurations working. A graph with a
+ * storage engine manages its own data and is skipped.
+ */
+ private static void loadCredentialStore(final TinkerGraph graph) {
Review Comment:
Out of scope for this PR, but I feel like SimpleAuthenticator should be
completely reworked for TinkerPop 4. It was already feeling dated, but
stitching it to removed Graph config options really emphasizes that further.
> TinkerGraph file persistence
> ----------------------------
>
> Key: TINKERPOP-3282
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3282
> Project: TinkerPop
> Issue Type: Improvement
> Components: tinkergraph
> Affects Versions: 3.7.6
> Reporter: Stephen Mallette
> Assignee: Stephen Mallette
> Priority: Major
>
> Give TinkerGraph a basic file persistence option tied to its transactional
> usage so that it can support some more complex use cases.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
