You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "vkagamlyk (via GitHub)" <gi...@apache.org> on 2023/06/06 07:55:03 UTC
[GitHub] [tinkerpop] vkagamlyk opened a new pull request, #2087: [WIP] Native transactions implementation
vkagamlyk opened a new pull request, #2087:
URL: https://github.com/apache/tinkerpop/pull/2087
Draft implementation:
- CRUD with transactions
- Unit tests for `TinkerTransactionGraph`
- Gherkin test setup
- `TinkerHelper` refactoring
What next:
- Test fixes
- Handle for indices
- Performance tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] Cole-Greer commented on a diff in pull request #2087: [WIP] Native transactions implementation
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1260132124
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java:
##########
@@ -27,14 +27,7 @@
import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
+import java.util.*;
Review Comment:
Prefer not to use wildcard imports
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266754132
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerFactory.java:
##########
@@ -54,10 +51,21 @@ public static TinkerGraph createClassic() {
return g;
}
+ /**
+ * Create the "classic" graph with transaction support.
+ * @see #createClassic()
+ */
+ public static TinkerTransactionGraph createTxClassic() {
Review Comment:
is there any reason we explicitly need these "Tx" versions? perhaps we just leave the `generateClassic(AbstractTinkerGraph)` and if you want a transactional graph you gotta pass it in as a transactional implementation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: [WIP] Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1259042200
##########
gremlin-server/src/test/scripts/generate-all.groovy:
##########
@@ -36,7 +36,7 @@ globals << [hook : [
// things now, that test config would have been mixed in with release artifacts and there would have been ugly
// exclusions to make packaging work properly.
allowSetOfIdManager = { graph, idManagerFieldName, idManager ->
- java.lang.reflect.Field idManagerField = graph.class.getDeclaredField(idManagerFieldName)
+ java.lang.reflect.Field idManagerField = graph.class.getSuperclass().getDeclaredField(idManagerFieldName)
Review Comment:
yes, other providers should use own scripts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] Cole-Greer commented on pull request #2087: Native transactions implementation
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#issuecomment-1646260588
LGTM, thanks Valentyn it will be great to have this added support in TinkerGraph. VOTE +!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1267385241
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/Attachable.java:
##########
@@ -197,8 +197,10 @@ public static Optional<Edge> getEdge(final Attachable<Edge> attachableEdge, fina
final Iterator<Edge> edgeIterator = hostVertex.edges(Direction.OUT, attachableEdge.get().label());
while (edgeIterator.hasNext()) {
final Edge edge = edgeIterator.next();
- if (ElementHelper.areEqual(edge, baseEdge))
+ if (ElementHelper.areEqual(edge, baseEdge)) {
+ CloseableIterator.closeIterator(edgeIterator);
Review Comment:
Yes, this is a problem, but I think it should be solved also in 3.5/3.6.
I added [ticket](https://issues.apache.org/jira/browse/TINKERPOP-2970) to track this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266737413
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerElementContainer.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.tinkergraph.structure;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantLock;
+
+final class TinkerElementContainer<T extends TinkerElement> {
Review Comment:
won't call out any more javadoc items, but i think it's worth a review of new classes/new transaction methods in this PR to document them up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1267091542
##########
gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/GraphSONMessageSerializerV3Test.java:
##########
@@ -288,9 +288,9 @@ public void shouldSerializeToTreeJson() throws Exception {
Iterator<Vertex> vertexKeys = firstTree.keySet().iterator();
Map expectedValues = new HashMap<Integer, String>() {{
- put(3, "vadas");
- put(5, "lop");
- put(7, "josh");
+ put(2, "vadas");
Review Comment:
the main reason is the change in the processing order of the vertices. Because of this, the vertex properties were created in a different order and received different IDs. Property IDs are not defined anywhere so I left it as is
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266722607
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerGraph.java:
##########
@@ -0,0 +1,508 @@
+/*
+ * 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.tinkergraph.structure;
+
+import org.apache.commons.configuration2.Configuration;
+import org.apache.tinkerpop.gremlin.process.computer.GraphComputer;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Element;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.structure.io.Io;
+import org.apache.tinkerpop.gremlin.structure.io.IoCore;
+import org.apache.tinkerpop.gremlin.structure.io.graphson.GraphSONVersion;
+import org.apache.tinkerpop.gremlin.structure.io.gryo.GryoVersion;
+import org.apache.tinkerpop.gremlin.tinkergraph.process.computer.TinkerGraphComputer;
+import org.apache.tinkerpop.gremlin.tinkergraph.process.computer.TinkerGraphComputerView;
+import org.apache.tinkerpop.gremlin.tinkergraph.services.TinkerServiceRegistry;
+
+import java.io.File;
+import java.lang.reflect.InvocationTargetException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.stream.Stream;
+
+public abstract class AbstractTinkerGraph implements Graph {
Review Comment:
please add some javadoc for the class (though it might be nice to add some more throughout). also, missing some `final` designations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1269717032
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/AbstractGraphProvider.java:
##########
@@ -151,5 +151,7 @@ protected void readIntoGraph(final Graph graph, final String path) throws IOExce
final String dataFile = TestHelper.generateTempFileFromResource(graph.getClass(),
GryoResourceAccess.class, path.substring(path.lastIndexOf(File.separator) + 1), "", false).getAbsolutePath();
graph.traversal().io(dataFile).read().iterate();
+ if (graph.features().graph().supportsTransactions())
Review Comment:
addressed in https://github.com/apache/tinkerpop/pull/2087/commits/595d8273753c1d4bf3ed072cf9afd5215dcc0420
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266637757
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/GraphMigrator.java:
##########
@@ -37,7 +37,8 @@
*/
public final class GraphMigrator {
- private GraphMigrator() {}
+ private GraphMigrator() {
Review Comment:
nit: revert spacing changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266716835
##########
gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/GraphSONMessageSerializerV3Test.java:
##########
@@ -288,9 +288,9 @@ public void shouldSerializeToTreeJson() throws Exception {
Iterator<Vertex> vertexKeys = firstTree.keySet().iterator();
Map expectedValues = new HashMap<Integer, String>() {{
- put(3, "vadas");
- put(5, "lop");
- put(7, "josh");
+ put(2, "vadas");
Review Comment:
what made all ids change (i see you had to do this in other tests as well)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] xiazcy commented on a diff in pull request #2087: [WIP] Native transactions implementation
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1261654502
##########
docs/src/reference/implementations-tinkertransactiongraph.asciidoc:
##########
@@ -0,0 +1,57 @@
+////
+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.
+////
+[[tinkertransactiongraph-gremlin]]
Review Comment:
I believe this needs an entry in the `reference/index.asciidoc` file in order for it to compile in the site.
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -60,6 +60,54 @@ gremlin> g.union(V().has('name','vadas'),
See: link:https://issues.apache.org/jira/browse/TINKERPOP-2873[TINKERPOP-2873]
+==== Native transaction support
+
+===== Introduction
+
+Now, if you need to use transactions in TinkerPop, the only solution is to use the Neo4j plugin. Unfortunately, this plugin has not been updated for a long time and is only compatible with Neo4j version 3.4, which reached end of life in March 2020. TinkerGraph cannot be used as a replacement for other graphs that support transactions.
+
+===== How to start using new transaction graph
+
+By default Gremlin Server don't provide transaction compatible graph, so need to add new graph in server configuration yaml file
Review Comment:
```suggestion
By default Gremlin Server doesn't provide transaction compatible graph, so one needs to add a new graph in server configuration yaml file
```
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -60,6 +60,54 @@ gremlin> g.union(V().has('name','vadas'),
See: link:https://issues.apache.org/jira/browse/TINKERPOP-2873[TINKERPOP-2873]
+==== Native transaction support
+
+===== Introduction
+
+Now, if you need to use transactions in TinkerPop, the only solution is to use the Neo4j plugin. Unfortunately, this plugin has not been updated for a long time and is only compatible with Neo4j version 3.4, which reached end of life in March 2020. TinkerGraph cannot be used as a replacement for other graphs that support transactions.
+
+===== How to start using new transaction graph
+
+By default Gremlin Server don't provide transaction compatible graph, so need to add new graph in server configuration yaml file
+[source,yaml]
+----
+ ttx: conf/tinkertransactiongraph-service.properties
+----
+
+and init graph in groovy script
+[source,groovy]
+----
+def dynamicGttx = context.getBindings(javax.script.ScriptContext.GLOBAL_SCOPE)["ttx"]
+if (dynamicGttx != null)
+ globals << [gttx : traversal().withEmbedded(dynamicGttx)]
+----
+
+===== Usage examples
+
+[source,java]
+----
+GraphTraversalSource g = traversal().withRemote(conn); <1>
+
+GraphTraversalSource gtx = g.tx().begin(); <2>
+try {
+ gtx.addV('test1').iterate(); <3>
+ gtx.addV('test2').iterate(); <3>
+
+ gtx.tx().commit(); <4>
+} catch (Exception ex) {
+ gtx.tx().rollback(); <5>
+}
+----
+
+<1> create connection to Gremlin Server with transaction enabled graph.
+<2> spawn a GraphTraversalSource with opened transaction.
+<3> made some updates to graph.
+<4> commit all.
+<5> rollback all changes on error.
Review Comment:
```suggestion
<1> Create connection to Gremlin Server with transaction enabled graph.
<2> Spawn a GraphTraversalSource with opened transaction.
<3> Make some updates to graph.
<4> Commit all changes.
<5> Rollback all changes on error.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk merged pull request #2087: Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk merged PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] Cole-Greer commented on a diff in pull request #2087: [WIP] Native transactions implementation
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1259029655
##########
gremlin-server/src/test/scripts/generate-all.groovy:
##########
@@ -36,7 +36,7 @@ globals << [hook : [
// things now, that test config would have been mixed in with release artifacts and there would have been ugly
// exclusions to make packaging work properly.
allowSetOfIdManager = { graph, idManagerFieldName, idManager ->
- java.lang.reflect.Field idManagerField = graph.class.getDeclaredField(idManagerFieldName)
+ java.lang.reflect.Field idManagerField = graph.class.getSuperclass().getDeclaredField(idManagerFieldName)
Review Comment:
I'm not very familiar with this script, is `graph` in this context guaranteed to be an instance of either TinkerGraph or TinkerTransactionGraph? Is it possible that this may be used by other providers and inserting `getSuperclass()` will break their testing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] Cole-Greer commented on a diff in pull request #2087: [WIP] Native transactions implementation
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1259032242
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/algorithm/generator/AbstractGeneratorTest.java:
##########
@@ -49,6 +49,7 @@ protected boolean same(final Graph g1, final Graph g2) {
final Iterator<Vertex> innerItty = g2.vertices();
final Vertex v = IteratorUtils.filter(innerItty, vx -> vx.value("oid").equals(p.getValue0())).next();
CloseableIterator.closeIterator(innerItty);
+ final boolean b = sameInVertices(v, p.getValue1()) && sameOutVertices(v, p.getValue2());
Review Comment:
Is this variable ever used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] Cole-Greer commented on a diff in pull request #2087: [WIP] Native transactions implementation
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1260038743
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/FeatureSupportTest.java:
##########
@@ -121,7 +121,8 @@ public void shouldSupportComputerIfAGraphCanCompute() throws Exception {
*/
@Test
@FeatureRequirement(featureClass = GraphFeatures.class, feature = FEATURE_TRANSACTIONS, supported = false)
- public void shouldSupportTransactionsIfAGraphConstructsATx() throws Exception {
+ @FeatureRequirement(featureClass = GraphFeatures.class, feature = FEATURE_THREADED_TRANSACTIONS, supported = false)
+ public void shouldSupportTransactionsIfAGraphConstructsATx() {
try {
graph.tx();
fail(String.format(INVALID_FEATURE_SPECIFICATION, GraphFeatures.class.getSimpleName(), FEATURE_TRANSACTIONS));
Review Comment:
Should `FEATURE_TRANSACTIONS` be updated here to `FEATURE_THREADED_TRANSACTIONS`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] Cole-Greer commented on a diff in pull request #2087: [WIP] Native transactions implementation
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1260134703
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java:
##########
@@ -43,17 +36,40 @@
public final class TinkerVertex extends TinkerElement implements Vertex {
protected Map<String, List<VertexProperty>> properties;
- protected Map<String, Set<Edge>> outEdges;
- protected Map<String, Set<Edge>> inEdges;
- private final TinkerGraph graph;
+ protected Map<String, Set<Object>> outEdges;
Review Comment:
Do the edge maps need to be relaxed to Object types here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1267058419
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/CollectionUtil.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.util;
+
+import java.util.ArrayList;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public final class CollectionUtil {
Review Comment:
consolidated in https://github.com/apache/tinkerpop/pull/2087/commits/9b6991dd123267ae5ffa536cb608e81d4c999aad
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266709441
##########
gremlin-server/src/test/scripts/test-server-start.groovy:
##########
@@ -60,7 +60,10 @@ settings.graphs.modern = platformAgnosticGremlinServerDir + "/src/test/scripts/t
settings.graphs.crew = platformAgnosticGremlinServerDir + "/src/test/scripts/tinkergraph-empty.properties"
settings.graphs.grateful = platformAgnosticGremlinServerDir + "/src/test/scripts/tinkergraph-empty.properties"
settings.graphs.sink = platformAgnosticGremlinServerDir + "/src/test/scripts/tinkergraph-empty.properties"
-if (testTransactions) settings.graphs.tx = platformAgnosticGremlinServerDir + "/src/test/scripts/neo4j-empty.properties"
+if (testTransactions) {
+ settings.graphs.tx = platformAgnosticGremlinServerDir + "/src/test/scripts/neo4j-empty.properties"
Review Comment:
is there any point to configuring neo4j for testing now that we have TinkerGraph? can't we just remove the configuration? iirc we only have neo4j here to validate that remote transactions work on the server and with drivers and we can now do that with TinkerGraph. we really don't need to test that neo4j itself works in this context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1267415278
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransactionGraph.java:
##########
@@ -0,0 +1,499 @@
+/*
+ * 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.tinkergraph.structure;
+
+import org.apache.commons.configuration2.BaseConfiguration;
+import org.apache.commons.configuration2.Configuration;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Element;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.ElementHelper;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.tinkergraph.process.traversal.strategy.optimization.TinkerGraphCountStrategy;
+import org.apache.tinkerpop.gremlin.tinkergraph.process.traversal.strategy.optimization.TinkerGraphStepStrategy;
+import org.apache.tinkerpop.gremlin.tinkergraph.services.TinkerServiceRegistry;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * An in-memory (with optional persistence on calls to {@link #close()}), reference implementation of the property
+ * graph interfaces with transaction support provided by TinkerPop.
+ *
+ * @author Valentyn Kahamlyk
+ */
+@Graph.OptIn(Graph.OptIn.SUITE_STRUCTURE_STANDARD)
+@Graph.OptIn(Graph.OptIn.SUITE_STRUCTURE_INTEGRATE)
+@Graph.OptIn(Graph.OptIn.SUITE_PROCESS_STANDARD)
+@Graph.OptIn(Graph.OptIn.SUITE_PROCESS_COMPUTER)
+@Graph.OptIn(Graph.OptIn.SUITE_PROCESS_LIMITED_STANDARD)
+@Graph.OptIn(Graph.OptIn.SUITE_PROCESS_LIMITED_COMPUTER)
+public final class TinkerTransactionGraph extends AbstractTinkerGraph {
Review Comment:
I think technically it's possible, but
1. Internal implementation related to elements storage is quite different, so overrided code will be a mess.
2. I think at some point difference between `TinkerGraph` and `TinkerTransactionGraph` can become bigger, for example for performance is better to use different structures for Vertex and Edge, not same like now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266637757
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/GraphMigrator.java:
##########
@@ -37,7 +37,8 @@
*/
public final class GraphMigrator {
- private GraphMigrator() {}
+ private GraphMigrator() {
Review Comment:
nit: revert spacing changes throughout this class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266692851
##########
docs/src/reference/implementations-tinkertransactiongraph.asciidoc:
##########
@@ -0,0 +1,57 @@
+////
+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.
+////
+[[tinkertransactiongraph-gremlin]]
+== TinkerTransactionGraph-Gremlin
Review Comment:
I don't think you should create a section at this level for this. i think this should be positioned as a "configuration" of TinkerGraph and not some additional/new `Graph` implementation. Simply add a section called "Transactions" to the `implementations-tinkergraph.asciidoc` and include your content. I think that what's written here needs some additional things:
1. "It is not suitable for production" - i don't think that's exactly the right way to present this message. I think it would be better to explain the transaction model that was implemented, describe the limitations it has and why those limitations would likely inhibit production use. At that point you can lean into the use cases it does have (i.e. testing).
2. Describe the testing use case clearly. Assume the reader knows nothing about how/why you would use TinkerGraph for testing in the first place.
3. "In other cases better to use TinkerGraph." - as mentioned earlier, avoid statements that make it sound like there are two "TinkerGraphs". We have one TinkerGraph. If you want to use transactions you use one class and if you don't want transaction support you use a different class. it's still just TinkerGraph.
4. I recommend examples use the embedded form rather than the remote form.
5. In Gremlin Server / Best Practices add a "Testing Remote Providers" and fully explain and demonstrate the testing use case with TinkerGraph.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: [WIP] Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1260164035
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/FeatureSupportTest.java:
##########
@@ -121,7 +121,8 @@ public void shouldSupportComputerIfAGraphCanCompute() throws Exception {
*/
@Test
@FeatureRequirement(featureClass = GraphFeatures.class, feature = FEATURE_TRANSACTIONS, supported = false)
- public void shouldSupportTransactionsIfAGraphConstructsATx() throws Exception {
+ @FeatureRequirement(featureClass = GraphFeatures.class, feature = FEATURE_THREADED_TRANSACTIONS, supported = false)
+ public void shouldSupportTransactionsIfAGraphConstructsATx() {
try {
graph.tx();
fail(String.format(INVALID_FEATURE_SPECIFICATION, GraphFeatures.class.getSimpleName(), FEATURE_TRANSACTIONS));
Review Comment:
no, `FEATURE_THREADED_TRANSACTIONS` means transaction can be executed across multiple threads
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: [WIP] Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1260142197
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java:
##########
@@ -43,17 +36,40 @@
public final class TinkerVertex extends TinkerElement implements Vertex {
protected Map<String, List<VertexProperty>> properties;
- protected Map<String, Set<Edge>> outEdges;
- protected Map<String, Set<Edge>> inEdges;
- private final TinkerGraph graph;
+ protected Map<String, Set<Object>> outEdges;
Review Comment:
yes, because map stores edge id
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] codecov-commenter commented on pull request #2087: [WIP] Native transactions implementation
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#issuecomment-1578148731
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2087?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#2087](https://app.codecov.io/gh/apache/tinkerpop/pull/2087?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (72083c9) into [master](https://app.codecov.io/gh/apache/tinkerpop/commit/6a62327d1cb82b8abfc6080e31866212c09a0fb7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6a62327) will **decrease** coverage by `4.50%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #2087 +/- ##
============================================
- Coverage 74.59% 70.10% -4.50%
============================================
Files 1026 24 -1002
Lines 62321 3532 -58789
Branches 6844 0 -6844
============================================
- Hits 46490 2476 -44014
+ Misses 13277 884 -12393
+ Partials 2554 172 -2382
```
[see 1003 files with indirect coverage changes](https://app.codecov.io/gh/apache/tinkerpop/pull/2087/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: [WIP] Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1259039588
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/algorithm/generator/AbstractGeneratorTest.java:
##########
@@ -49,6 +49,7 @@ protected boolean same(final Graph g1, final Graph g2) {
final Iterator<Vertex> innerItty = g2.vertices();
final Vertex v = IteratorUtils.filter(innerItty, vx -> vx.value("oid").equals(p.getValue0())).next();
CloseableIterator.closeIterator(innerItty);
+ final boolean b = sameInVertices(v, p.getValue1()) && sameOutVertices(v, p.getValue2());
Review Comment:
it was debug code, removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266723017
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerIndex.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.tinkergraph.structure;
+
+import org.apache.tinkerpop.gremlin.structure.Element;
+
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+public abstract class AbstractTinkerIndex<T extends Element> {
Review Comment:
javadoc please...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1267506076
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/StructureStandardSuite.java:
##########
@@ -105,6 +105,7 @@ public class StructureStandardSuite extends AbstractGremlinSuite {
SerializationTest.class,
StarGraphTest.class,
TransactionTest.class,
+ TransactionTestV2.class,
Review Comment:
I renamed it and added explanation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266630726
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/StructureStandardSuite.java:
##########
@@ -105,6 +105,7 @@ public class StructureStandardSuite extends AbstractGremlinSuite {
SerializationTest.class,
StarGraphTest.class,
TransactionTest.class,
+ TransactionTestV2.class,
Review Comment:
could you please explain the difference between these transaction tests and the old transaction tests? and why can't neo4j support these? will other graphs not be able to support these (i.e. i see it's got an `OptOut`)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266648419
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/CollectionUtil.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.util;
+
+import java.util.ArrayList;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public final class CollectionUtil {
Review Comment:
there is a `org.apache.tinkerpop.gremlin.util.tools` with a `CollectionFactory`. should we move the functions there to this class and remove `CollectionFactory`? i think this is the better named class. may as well get rid of "tools" package and just move `MultiMap` up a level to this package. please `final` variables in this class and add unit tests for `clone()` - any reason the method uses a capital "C" and not lower case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on pull request #2087: Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#issuecomment-1646245516
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266667344
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -60,6 +60,54 @@ gremlin> g.union(V().has('name','vadas'),
See: link:https://issues.apache.org/jira/browse/TINKERPOP-2873[TINKERPOP-2873]
+==== Native transaction support
Review Comment:
A few things regarding the upgrade docs:
1. I think you should title this as "TinkerGraph Transactions" because we didn't really implemented "native transactions" in TinkerPop to apply to any graph. They are transactions specific to TinkerGraph itself as a reference implementation.
2. I'm not sure what's written here is telling the right story for the Upgrade Documentation. It's sort of focused on Gremlin Server with examples that really don't have much to do with the specific change that you implemented. I think you want to explain (a) that TinkerGraph has transaction functionality now, (b) summarize its capability briefly, (c) include Console session output for how to create a transactional TinkerGraph, add a vertex, commit/rollback (d) explain when best to use it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a diff in pull request #2087: Native transactions implementation
Posted by "mikepersonick (via GitHub)" <gi...@apache.org>.
mikepersonick commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1261185479
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/Attachable.java:
##########
@@ -271,8 +273,10 @@ public static Optional<Property> getProperty(final Attachable<Property> attachab
final Edge edge = edgeIterator.next();
if (ElementHelper.areEqual(edge, propertyElement)) {
final Property property = edge.property(baseProperty.key());
- if (ElementHelper.areEqual(baseProperty, property))
+ if (ElementHelper.areEqual(baseProperty, property)) {
+ CloseableIterator.closeIterator(edgeIterator);
Review Comment:
same
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/Attachable.java:
##########
@@ -282,6 +286,7 @@ public static Optional<Property> getProperty(final Attachable<Property> attachab
final VertexProperty vertexProperty = vertexPropertyIterator.next();
if (ElementHelper.areEqual(vertexProperty, baseProperty.element())) {
final Property property = vertexProperty.property(baseProperty.key());
+ CloseableIterator.closeIterator(vertexPropertyIterator);
Review Comment:
same
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/Attachable.java:
##########
@@ -197,8 +197,10 @@ public static Optional<Edge> getEdge(final Attachable<Edge> attachableEdge, fina
final Iterator<Edge> edgeIterator = hostVertex.edges(Direction.OUT, attachableEdge.get().label());
while (edgeIterator.hasNext()) {
final Edge edge = edgeIterator.next();
- if (ElementHelper.areEqual(edge, baseEdge))
+ if (ElementHelper.areEqual(edge, baseEdge)) {
+ CloseableIterator.closeIterator(edgeIterator);
Review Comment:
Why not close the iterator if the edge is not found as well? We get a lot of resource leaks in Neptune bc of open source laziness around cleaning up iterators.
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransaction.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * 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.tinkergraph.structure;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.AbstractThreadLocalTransaction;
+import org.apache.tinkerpop.gremlin.structure.util.TransactionException;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicLong;
+
+final class TinkerTransaction extends AbstractThreadLocalTransaction {
+
+ private static final String TX_CONFLICT = "Conflict: element modified in another transaction";
+
+ private static final long NOT_STARTED = -1;
+
+ private static final AtomicLong openedTx;
+ private final ThreadLocal<Long> txNumber = ThreadLocal.withInitial(() -> NOT_STARTED);
+ private final ThreadLocal<Set<TinkerElementContainer<TinkerVertex>>> txChangedVertices = new ThreadLocal<>();
+ private final ThreadLocal<Set<TinkerElementContainer<TinkerEdge>>> txChangedEdges = new ThreadLocal<>();
+
+ private final TinkerTransactionGraph graph;
+
+ static {
+ openedTx = new AtomicLong(0);
+ }
+
+ public TinkerTransaction(final TinkerTransactionGraph g) {
+ super(g);
+ graph = g;
+ }
+
+ @Override
+ public boolean isOpen() {
+ return txNumber.get() != NOT_STARTED;
+ }
+
+ @Override
+ public <T extends TraversalSource> T begin() {
+ doOpen();
+ return super.begin();
+ }
+
+ @Override
+ protected void doOpen() {
+ if (isOpen())
+ Transaction.Exceptions.transactionAlreadyOpen();
+
+ txNumber.set(openedTx.getAndIncrement());
+ }
+
+ protected long getTxNumber() {
+ if (!isOpen()) txNumber.set(openedTx.getAndIncrement());
+ return txNumber.get();
+ }
+
+ protected <T extends TinkerElement> void touch(TinkerElementContainer<T> container) {
+ if (!isOpen()) txNumber.set(openedTx.getAndIncrement());
+
+ T element = container.getUnmodified();
+ if (null == element) element = container.getModified();
+ if (element instanceof TinkerVertex) {
+ if (null == txChangedVertices.get())
+ txChangedVertices.set(new HashSet<>());
+ txChangedVertices.get().add((TinkerElementContainer<TinkerVertex>) container);
+ } else {
+ if (null == txChangedEdges.get())
+ txChangedEdges.set(new HashSet<>());
+ txChangedEdges.get().add((TinkerElementContainer<TinkerEdge>) container);
+ }
+ }
+
+ @Override
+ protected void doCommit() throws TransactionException {
Review Comment:
I think we need a narrative here (Javadoc) explaining the thought process and approach. It's a bit tough to decipher just looking at the code and it will be hard for others to work on this code later.
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransaction.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * 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.tinkergraph.structure;
+
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.util.AbstractThreadLocalTransaction;
+import org.apache.tinkerpop.gremlin.structure.util.TransactionException;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicLong;
+
+final class TinkerTransaction extends AbstractThreadLocalTransaction {
+
+ private static final String TX_CONFLICT = "Conflict: element modified in another transaction";
+
+ private static final long NOT_STARTED = -1;
+
+ private static final AtomicLong openedTx;
+ private final ThreadLocal<Long> txNumber = ThreadLocal.withInitial(() -> NOT_STARTED);
+ private final ThreadLocal<Set<TinkerElementContainer<TinkerVertex>>> txChangedVertices = new ThreadLocal<>();
+ private final ThreadLocal<Set<TinkerElementContainer<TinkerEdge>>> txChangedEdges = new ThreadLocal<>();
+
+ private final TinkerTransactionGraph graph;
+
+ static {
+ openedTx = new AtomicLong(0);
+ }
+
+ public TinkerTransaction(final TinkerTransactionGraph g) {
+ super(g);
+ graph = g;
+ }
+
+ @Override
+ public boolean isOpen() {
+ return txNumber.get() != NOT_STARTED;
+ }
+
+ @Override
+ public <T extends TraversalSource> T begin() {
+ doOpen();
+ return super.begin();
+ }
+
+ @Override
+ protected void doOpen() {
+ if (isOpen())
+ Transaction.Exceptions.transactionAlreadyOpen();
+
+ txNumber.set(openedTx.getAndIncrement());
+ }
+
+ protected long getTxNumber() {
+ if (!isOpen()) txNumber.set(openedTx.getAndIncrement());
+ return txNumber.get();
+ }
+
+ protected <T extends TinkerElement> void touch(TinkerElementContainer<T> container) {
+ if (!isOpen()) txNumber.set(openedTx.getAndIncrement());
+
+ T element = container.getUnmodified();
+ if (null == element) element = container.getModified();
+ if (element instanceof TinkerVertex) {
+ if (null == txChangedVertices.get())
+ txChangedVertices.set(new HashSet<>());
+ txChangedVertices.get().add((TinkerElementContainer<TinkerVertex>) container);
+ } else {
+ if (null == txChangedEdges.get())
+ txChangedEdges.set(new HashSet<>());
+ txChangedEdges.get().add((TinkerElementContainer<TinkerEdge>) container);
+ }
+ }
+
+ @Override
+ protected void doCommit() throws TransactionException {
+ final long txVersion = txNumber.get();
+
+ Set<TinkerElementContainer<TinkerVertex>> changedVertices = txChangedVertices.get();
+ if (null == changedVertices) changedVertices = Collections.emptySet();
+ Set<TinkerElementContainer<TinkerEdge>> changedEdges = txChangedEdges.get();
+ if (null == changedEdges) changedEdges = Collections.emptySet();
+
+ try {
+ // Double-checked locking to reduce lock time
+ if (changedVertices.stream().anyMatch(v -> v.updatedOutsideTransaction()) ||
+ changedEdges.stream().anyMatch(v -> v.updatedOutsideTransaction()))
+ throw new TransactionException(TX_CONFLICT);
+
+ changedVertices.forEach(v -> {
+ if (!v.tryLock()) throw new TransactionException(TX_CONFLICT);
+ });
+ changedEdges.forEach(e -> {
+ if (!e.tryLock()) throw new TransactionException(TX_CONFLICT);
+ });
+
+ if (changedVertices.stream().anyMatch(v -> v.updatedOutsideTransaction()) ||
+ changedEdges.stream().anyMatch(e -> e.updatedOutsideTransaction()))
+ throw new TransactionException(TX_CONFLICT);
+
+ final TinkerTransactionalIndex vertexIndex = (TinkerTransactionalIndex) graph.vertexIndex;
+ if (vertexIndex != null) vertexIndex.commit(changedVertices);
+ final TinkerTransactionalIndex edgeIndex = (TinkerTransactionalIndex) graph.edgeIndex;
+ if (edgeIndex != null) edgeIndex.commit(changedEdges);
+
+ changedVertices.forEach(v -> v.commit(txVersion));
+ changedEdges.forEach(e -> e.commit(txVersion));
+ } catch (TransactionException ex) {
+ changedVertices.forEach(v -> v.rollback());
+ changedEdges.forEach(e -> e.rollback());
+
+ final TinkerTransactionalIndex vertexIndex = (TinkerTransactionalIndex) graph.vertexIndex;
+ if (vertexIndex != null) vertexIndex.rollback();
+ final TinkerTransactionalIndex edgeIndex = (TinkerTransactionalIndex) graph.edgeIndex;
+ if (edgeIndex != null) edgeIndex.rollback();
+
+ throw ex;
+ } finally {
+ // remove elements from graph if not used in other tx's
+ changedVertices.stream().filter(v -> v.canBeRemoved()).forEach(v -> graph.vertices.remove(v.getElementId()));
+ changedEdges.stream().filter(e -> e.canBeRemoved()).forEach(e -> graph.edges.remove(e.getElementId()));
+
+ txChangedVertices.remove();
+ txChangedEdges.remove();
+
+ changedVertices.forEach(v -> v.releaseLock());
+ changedEdges.forEach(e -> e.releaseLock());
+
+ txNumber.set(NOT_STARTED);
+ }
+ }
+
+ @Override
+ protected void doRollback() throws TransactionException {
Review Comment:
same as above
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransactionalIndex.java:
##########
@@ -0,0 +1,239 @@
+/*
+ * 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.tinkergraph.structure;
+
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+
+/**
+ * @author Marko A. Rodriguez (http://markorodriguez.com)
Review Comment:
?
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransactionGraph.java:
##########
@@ -0,0 +1,499 @@
+/*
+ * 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.tinkergraph.structure;
+
+import org.apache.commons.configuration2.BaseConfiguration;
+import org.apache.commons.configuration2.Configuration;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Element;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.ElementHelper;
+import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.tinkergraph.process.traversal.strategy.optimization.TinkerGraphCountStrategy;
+import org.apache.tinkerpop.gremlin.tinkergraph.process.traversal.strategy.optimization.TinkerGraphStepStrategy;
+import org.apache.tinkerpop.gremlin.tinkergraph.services.TinkerServiceRegistry;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * An in-memory (with optional persistence on calls to {@link #close()}), reference implementation of the property
+ * graph interfaces with transaction support provided by TinkerPop.
+ *
+ * @author Valentyn Kahamlyk
+ */
+@Graph.OptIn(Graph.OptIn.SUITE_STRUCTURE_STANDARD)
+@Graph.OptIn(Graph.OptIn.SUITE_STRUCTURE_INTEGRATE)
+@Graph.OptIn(Graph.OptIn.SUITE_PROCESS_STANDARD)
+@Graph.OptIn(Graph.OptIn.SUITE_PROCESS_COMPUTER)
+@Graph.OptIn(Graph.OptIn.SUITE_PROCESS_LIMITED_STANDARD)
+@Graph.OptIn(Graph.OptIn.SUITE_PROCESS_LIMITED_COMPUTER)
+public final class TinkerTransactionGraph extends AbstractTinkerGraph {
Review Comment:
It would be great if we could get rid of AbstractTinkerGraph. Just make TinkerGraph the base class and TinkerTransactionGraph extend it. Not sure if it's possible.
##########
tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransactionGraphTest.java:
##########
@@ -0,0 +1,722 @@
+/*
+ * 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.tinkergraph.structure;
+
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.util.TransactionException;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
+
+public class TinkerTransactionGraphTest {
Review Comment:
stress tests
##########
gremlin-server/src/test/scripts/generate-all.groovy:
##########
@@ -36,7 +36,7 @@ globals << [hook : [
// things now, that test config would have been mixed in with release artifacts and there would have been ugly
// exclusions to make packaging work properly.
allowSetOfIdManager = { graph, idManagerFieldName, idManager ->
- java.lang.reflect.Field idManagerField = graph.class.getDeclaredField(idManagerFieldName)
+ java.lang.reflect.Field idManagerField = graph.class.getSuperclass().getDeclaredField(idManagerFieldName)
Review Comment:
This is very brittle. class.getSuperclass().getDeclaredField() should be changed to class.getField(), which will find fields on the superclass hierarchy (if there is one)
##########
gremlin-server/src/test/scripts/test-server-start.groovy:
##########
@@ -60,7 +60,10 @@ settings.graphs.modern = platformAgnosticGremlinServerDir + "/src/test/scripts/t
settings.graphs.crew = platformAgnosticGremlinServerDir + "/src/test/scripts/tinkergraph-empty.properties"
settings.graphs.grateful = platformAgnosticGremlinServerDir + "/src/test/scripts/tinkergraph-empty.properties"
settings.graphs.sink = platformAgnosticGremlinServerDir + "/src/test/scripts/tinkergraph-empty.properties"
-if (testTransactions) settings.graphs.tx = platformAgnosticGremlinServerDir + "/src/test/scripts/neo4j-empty.properties"
+if (testTransactions) {
+ settings.graphs.tx = platformAgnosticGremlinServerDir + "/src/test/scripts/neo4j-empty.properties"
Review Comment:
Yes, part of this work should be pulling out the Neo4j integration
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/AbstractGraphProvider.java:
##########
@@ -151,5 +151,7 @@ protected void readIntoGraph(final Graph graph, final String path) throws IOExce
final String dataFile = TestHelper.generateTempFileFromResource(graph.getClass(),
GryoResourceAccess.class, path.substring(path.lastIndexOf(File.separator) + 1), "", false).getAbsolutePath();
graph.traversal().io(dataFile).read().iterate();
+ if (graph.features().graph().supportsTransactions())
Review Comment:
+1
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerIndex.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.tinkergraph.structure;
+
+import org.apache.tinkerpop.gremlin.structure.Element;
+
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+public abstract class AbstractTinkerIndex<T extends Element> {
Review Comment:
+1
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/algorithm/generator/AbstractGeneratorTest.java:
##########
@@ -58,11 +58,23 @@ protected boolean same(final Graph g1, final Graph g2) {
private boolean sameInVertices(final Vertex v, final List<Vertex> inVertices) {
return inVertices.stream()
- .allMatch(inVertex -> IteratorUtils.filter(v.vertices(Direction.IN), hv -> hv.value("oid").equals(inVertex.value("oid"))).hasNext());
+ .allMatch(inVertex ->
+ {
+ final Iterator<Vertex> itty = v.vertices(Direction.IN);
+ final boolean result = IteratorUtils.filter(itty, hv -> hv.value("oid").equals(inVertex.value("oid"))).hasNext();
+ CloseableIterator.closeIterator(itty);
+ return result;
+ });
}
private boolean sameOutVertices(final Vertex v, final List<Vertex> outVertices) {
return outVertices.stream()
- .allMatch(outVertex -> IteratorUtils.filter(v.vertices(Direction.OUT), hv -> hv.value("oid").equals(outVertex.value("oid"))).hasNext());
+ .allMatch(outVertex ->
+ {
+ final Iterator<Vertex> itty = v.vertices(Direction.OUT);
+ final boolean result = IteratorUtils.filter(itty, hv -> hv.value("oid").equals(outVertex.value("oid"))).hasNext();
+ CloseableIterator.closeIterator(itty);
Review Comment:
same
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerElementContainer.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.tinkergraph.structure;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantLock;
+
+final class TinkerElementContainer<T extends TinkerElement> {
Review Comment:
Same as the Transaction class, I think we need a narrative here (Javadoc) explaining the thought process and approach. It's a bit tough to decipher just looking at the code and it will be hard for others to work on this code later. markDeleted -> touch -> setDraft -> touch ... some explanation would go a long way.
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java:
##########
@@ -43,17 +36,40 @@
public final class TinkerVertex extends TinkerElement implements Vertex {
protected Map<String, List<VertexProperty>> properties;
- protected Map<String, Set<Edge>> outEdges;
- protected Map<String, Set<Edge>> inEdges;
- private final TinkerGraph graph;
+ protected Map<String, Set<Object>> outEdges;
Review Comment:
Can we document this in the code? I assume this is related to that test case change? Why was it necessary?
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/algorithm/generator/AbstractGeneratorTest.java:
##########
@@ -58,11 +58,23 @@ protected boolean same(final Graph g1, final Graph g2) {
private boolean sameInVertices(final Vertex v, final List<Vertex> inVertices) {
return inVertices.stream()
- .allMatch(inVertex -> IteratorUtils.filter(v.vertices(Direction.IN), hv -> hv.value("oid").equals(inVertex.value("oid"))).hasNext());
+ .allMatch(inVertex ->
+ {
+ final Iterator<Vertex> itty = v.vertices(Direction.IN);
+ final boolean result = IteratorUtils.filter(itty, hv -> hv.value("oid").equals(inVertex.value("oid"))).hasNext();
+ CloseableIterator.closeIterator(itty);
Review Comment:
Can we do this in a try/finally everywhere so we don't leak resources in exception cases?
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/TransactionTestV2.java:
##########
@@ -0,0 +1,671 @@
+/*
+ * 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.structure;
+
+import org.apache.tinkerpop.gremlin.AbstractGremlinTest;
+import org.apache.tinkerpop.gremlin.FeatureRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.util.TransactionException;
+import org.junit.Test;
+
+import static org.apache.tinkerpop.gremlin.structure.Graph.Features.EdgeFeatures.FEATURE_ADD_EDGES;
+import static org.apache.tinkerpop.gremlin.structure.Graph.Features.EdgeFeatures.FEATURE_REMOVE_EDGES;
+import static org.apache.tinkerpop.gremlin.structure.Graph.Features.ElementFeatures.FEATURE_ADD_PROPERTY;
+import static org.apache.tinkerpop.gremlin.structure.Graph.Features.GraphFeatures.FEATURE_TRANSACTIONS;
+import static org.apache.tinkerpop.gremlin.structure.Graph.Features.VertexFeatures.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+public class TransactionTestV2 extends AbstractGremlinTest {
Review Comment:
Either move these back into TransactionTest (if they are general tests that apply to any provider) or move them into the Tinkergraph module (if they are specific tests for your implementation). Either way, we also need a TinkerGraphTransactionStressTest that stress tests your implementation under higher concurrency. We didn't have stress tests for Neo4j and other provider transactions because we assumed that the provider already had those natively and we could trust that their transactions implementation worked - we can no longer assume that. Stress test meaning many threads trying to do many thing concurrently and making sure that the outcome is what we'd expect. That should probably be in the Tinkergraph module though.
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerHelper.java:
##########
@@ -53,176 +49,102 @@ public final class TinkerHelper {
private TinkerHelper() {
}
- protected static Edge addEdge(final TinkerGraph graph, final TinkerVertex outVertex, final TinkerVertex inVertex, final String label, final Object... keyValues) {
- ElementHelper.validateLabel(label);
- ElementHelper.legalPropertyKeyValueArray(keyValues);
-
- Object idValue = graph.edgeIdManager.convert(ElementHelper.getIdValue(keyValues).orElse(null));
-
- final Edge edge;
- if (null != idValue) {
- if (graph.edges.containsKey(idValue))
- throw Graph.Exceptions.edgeWithIdAlreadyExists(idValue);
- } else {
- idValue = graph.edgeIdManager.getNextId(graph);
- }
-
- edge = new TinkerEdge(idValue, outVertex, label, inVertex);
- ElementHelper.attachProperties(edge, keyValues);
- graph.edges.put(edge.id(), edge);
- TinkerHelper.addOutEdge(outVertex, label, edge);
- TinkerHelper.addInEdge(inVertex, label, edge);
- return edge;
-
- }
-
- protected static void addOutEdge(final TinkerVertex vertex, final String label, final Edge edge) {
- if (null == vertex.outEdges) vertex.outEdges = new HashMap<>();
- Set<Edge> edges = vertex.outEdges.get(label);
- if (null == edges) {
- edges = new HashSet<>();
- vertex.outEdges.put(label, edges);
- }
- edges.add(edge);
- }
-
- protected static void addInEdge(final TinkerVertex vertex, final String label, final Edge edge) {
- if (null == vertex.inEdges) vertex.inEdges = new HashMap<>();
- Set<Edge> edges = vertex.inEdges.get(label);
- if (null == edges) {
- edges = new HashSet<>();
- vertex.inEdges.put(label, edges);
- }
- edges.add(edge);
- }
-
- public static List<TinkerVertex> queryVertexIndex(final TinkerGraph graph, final String key, final Object value) {
- return null == graph.vertexIndex ? Collections.emptyList() : graph.vertexIndex.get(key, value);
- }
-
- public static List<TinkerEdge> queryEdgeIndex(final TinkerGraph graph, final String key, final Object value) {
- return null == graph.edgeIndex ? Collections.emptyList() : graph.edgeIndex.get(key, value);
+ public static Map<String, List<VertexProperty>> getProperties(final TinkerVertex vertex) {
+ return null == vertex.properties ? Collections.emptyMap() : vertex.properties;
}
- public static boolean inComputerMode(final TinkerGraph graph) {
+ // todo: move to graph?
+ public static boolean inComputerMode(final AbstractTinkerGraph graph) {
return null != graph.graphComputerView;
}
- public static TinkerGraphComputerView createGraphComputerView(final TinkerGraph graph, final GraphFilter graphFilter, final Set<VertexComputeKey> computeKeys) {
+ public static TinkerGraphComputerView createGraphComputerView(final AbstractTinkerGraph graph, final GraphFilter graphFilter, final Set<VertexComputeKey> computeKeys) {
return graph.graphComputerView = new TinkerGraphComputerView(graph, graphFilter, computeKeys);
}
- public static TinkerGraphComputerView getGraphComputerView(final TinkerGraph graph) {
+ public static TinkerGraphComputerView getGraphComputerView(final AbstractTinkerGraph graph) {
return graph.graphComputerView;
}
- public static void dropGraphComputerView(final TinkerGraph graph) {
+ public static void dropGraphComputerView(final AbstractTinkerGraph graph) {
graph.graphComputerView = null;
}
- public static Map<String, List<VertexProperty>> getProperties(final TinkerVertex vertex) {
- return null == vertex.properties ? Collections.emptyMap() : vertex.properties;
- }
-
- public static void autoUpdateIndex(final TinkerEdge edge, final String key, final Object newValue, final Object oldValue) {
- final TinkerGraph graph = (TinkerGraph) edge.graph();
- if (graph.edgeIndex != null)
- graph.edgeIndex.autoUpdate(key, newValue, oldValue, edge);
- }
-
- public static void autoUpdateIndex(final TinkerVertex vertex, final String key, final Object newValue, final Object oldValue) {
- final TinkerGraph graph = (TinkerGraph) vertex.graph();
- if (graph.vertexIndex != null)
- graph.vertexIndex.autoUpdate(key, newValue, oldValue, vertex);
- }
-
- public static void removeElementIndex(final TinkerVertex vertex) {
- final TinkerGraph graph = (TinkerGraph) vertex.graph();
- if (graph.vertexIndex != null)
- graph.vertexIndex.removeElement(vertex);
- }
-
- public static void removeElementIndex(final TinkerEdge edge) {
- final TinkerGraph graph = (TinkerGraph) edge.graph();
- if (graph.edgeIndex != null)
- graph.edgeIndex.removeElement(edge);
- }
-
- public static void removeIndex(final TinkerVertex vertex, final String key, final Object value) {
- final TinkerGraph graph = (TinkerGraph) vertex.graph();
- if (graph.vertexIndex != null)
- graph.vertexIndex.remove(key, value, vertex);
- }
-
- public static void removeIndex(final TinkerEdge edge, final String key, final Object value) {
- final TinkerGraph graph = (TinkerGraph) edge.graph();
- if (graph.edgeIndex != null)
- graph.edgeIndex.remove(key, value, edge);
- }
-
+ // todo: move to TinkerVertex?
public static Iterator<TinkerEdge> getEdges(final TinkerVertex vertex, final Direction direction, final String... edgeLabels) {
Review Comment:
Why were these changes necessary?
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/StructureStandardSuite.java:
##########
@@ -105,6 +105,7 @@ public class StructureStandardSuite extends AbstractGremlinSuite {
SerializationTest.class,
StarGraphTest.class,
TransactionTest.class,
+ TransactionTestV2.class,
Review Comment:
If you needed additional test coverage just add it to TransactionTest. "V2" is not a good name for a test.
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/CollectionUtil.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.util;
+
+import java.util.ArrayList;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public final class CollectionUtil {
Review Comment:
Clone should be lower case for sure. I'm for consolidation of those items as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2087: Native transactions implementation
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1266713404
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/AbstractGraphProvider.java:
##########
@@ -151,5 +151,7 @@ protected void readIntoGraph(final Graph graph, final String path) throws IOExce
final String dataFile = TestHelper.generateTempFileFromResource(graph.getClass(),
GryoResourceAccess.class, path.substring(path.lastIndexOf(File.separator) + 1), "", false).getAbsolutePath();
graph.traversal().io(dataFile).read().iterate();
+ if (graph.features().graph().supportsTransactions())
Review Comment:
i'm pretty sure we left this code out purposefully and meant for the graph to handle commit after read on its own. unless there's a good reason to change this, i'd say it would be better to move this code to TinkerGraph's test implementation and just add to the javadoc to note that implementers should handle commit on their own.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1267505697
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerHelper.java:
##########
@@ -53,176 +49,102 @@ public final class TinkerHelper {
private TinkerHelper() {
}
- protected static Edge addEdge(final TinkerGraph graph, final TinkerVertex outVertex, final TinkerVertex inVertex, final String label, final Object... keyValues) {
- ElementHelper.validateLabel(label);
- ElementHelper.legalPropertyKeyValueArray(keyValues);
-
- Object idValue = graph.edgeIdManager.convert(ElementHelper.getIdValue(keyValues).orElse(null));
-
- final Edge edge;
- if (null != idValue) {
- if (graph.edges.containsKey(idValue))
- throw Graph.Exceptions.edgeWithIdAlreadyExists(idValue);
- } else {
- idValue = graph.edgeIdManager.getNextId(graph);
- }
-
- edge = new TinkerEdge(idValue, outVertex, label, inVertex);
- ElementHelper.attachProperties(edge, keyValues);
- graph.edges.put(edge.id(), edge);
- TinkerHelper.addOutEdge(outVertex, label, edge);
- TinkerHelper.addInEdge(inVertex, label, edge);
- return edge;
-
- }
-
- protected static void addOutEdge(final TinkerVertex vertex, final String label, final Edge edge) {
- if (null == vertex.outEdges) vertex.outEdges = new HashMap<>();
- Set<Edge> edges = vertex.outEdges.get(label);
- if (null == edges) {
- edges = new HashSet<>();
- vertex.outEdges.put(label, edges);
- }
- edges.add(edge);
- }
-
- protected static void addInEdge(final TinkerVertex vertex, final String label, final Edge edge) {
- if (null == vertex.inEdges) vertex.inEdges = new HashMap<>();
- Set<Edge> edges = vertex.inEdges.get(label);
- if (null == edges) {
- edges = new HashSet<>();
- vertex.inEdges.put(label, edges);
- }
- edges.add(edge);
- }
-
- public static List<TinkerVertex> queryVertexIndex(final TinkerGraph graph, final String key, final Object value) {
- return null == graph.vertexIndex ? Collections.emptyList() : graph.vertexIndex.get(key, value);
- }
-
- public static List<TinkerEdge> queryEdgeIndex(final TinkerGraph graph, final String key, final Object value) {
- return null == graph.edgeIndex ? Collections.emptyList() : graph.edgeIndex.get(key, value);
+ public static Map<String, List<VertexProperty>> getProperties(final TinkerVertex vertex) {
+ return null == vertex.properties ? Collections.emptyMap() : vertex.properties;
}
- public static boolean inComputerMode(final TinkerGraph graph) {
+ // todo: move to graph?
+ public static boolean inComputerMode(final AbstractTinkerGraph graph) {
return null != graph.graphComputerView;
}
- public static TinkerGraphComputerView createGraphComputerView(final TinkerGraph graph, final GraphFilter graphFilter, final Set<VertexComputeKey> computeKeys) {
+ public static TinkerGraphComputerView createGraphComputerView(final AbstractTinkerGraph graph, final GraphFilter graphFilter, final Set<VertexComputeKey> computeKeys) {
return graph.graphComputerView = new TinkerGraphComputerView(graph, graphFilter, computeKeys);
}
- public static TinkerGraphComputerView getGraphComputerView(final TinkerGraph graph) {
+ public static TinkerGraphComputerView getGraphComputerView(final AbstractTinkerGraph graph) {
return graph.graphComputerView;
}
- public static void dropGraphComputerView(final TinkerGraph graph) {
+ public static void dropGraphComputerView(final AbstractTinkerGraph graph) {
graph.graphComputerView = null;
}
- public static Map<String, List<VertexProperty>> getProperties(final TinkerVertex vertex) {
- return null == vertex.properties ? Collections.emptyMap() : vertex.properties;
- }
-
- public static void autoUpdateIndex(final TinkerEdge edge, final String key, final Object newValue, final Object oldValue) {
- final TinkerGraph graph = (TinkerGraph) edge.graph();
- if (graph.edgeIndex != null)
- graph.edgeIndex.autoUpdate(key, newValue, oldValue, edge);
- }
-
- public static void autoUpdateIndex(final TinkerVertex vertex, final String key, final Object newValue, final Object oldValue) {
- final TinkerGraph graph = (TinkerGraph) vertex.graph();
- if (graph.vertexIndex != null)
- graph.vertexIndex.autoUpdate(key, newValue, oldValue, vertex);
- }
-
- public static void removeElementIndex(final TinkerVertex vertex) {
- final TinkerGraph graph = (TinkerGraph) vertex.graph();
- if (graph.vertexIndex != null)
- graph.vertexIndex.removeElement(vertex);
- }
-
- public static void removeElementIndex(final TinkerEdge edge) {
- final TinkerGraph graph = (TinkerGraph) edge.graph();
- if (graph.edgeIndex != null)
- graph.edgeIndex.removeElement(edge);
- }
-
- public static void removeIndex(final TinkerVertex vertex, final String key, final Object value) {
- final TinkerGraph graph = (TinkerGraph) vertex.graph();
- if (graph.vertexIndex != null)
- graph.vertexIndex.remove(key, value, vertex);
- }
-
- public static void removeIndex(final TinkerEdge edge, final String key, final Object value) {
- final TinkerGraph graph = (TinkerGraph) edge.graph();
- if (graph.edgeIndex != null)
- graph.edgeIndex.remove(key, value, edge);
- }
-
+ // todo: move to TinkerVertex?
public static Iterator<TinkerEdge> getEdges(final TinkerVertex vertex, final Direction direction, final String... edgeLabels) {
Review Comment:
I changed the way how vertices contains linked edges. The main reason is the ability to set a transaction boundary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] kenhuuu commented on a diff in pull request #2087: Native transactions implementation
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1267391531
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerFactory.java:
##########
@@ -54,10 +51,21 @@ public static TinkerGraph createClassic() {
return g;
}
+ /**
+ * Create the "classic" graph with transaction support.
+ * @see #createClassic()
+ */
+ public static TinkerTransactionGraph createTxClassic() {
Review Comment:
It was done this way so that users could easily get either a transaction version or regular version of TinkerGraph without having to supply their own graph instance. This helps to keep it easy to use. Also, since there is currently only two implementations, I felt it was OK to just add Tx versions rather than completely making separate Factory classes for each of them. Though, I would agree that it makes sense to change how this works in the future if an implementation was added or removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2087: Native transactions implementation
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1269845726
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/GraphMigrator.java:
##########
@@ -37,7 +37,8 @@
*/
public final class GraphMigrator {
- private GraphMigrator() {}
+ private GraphMigrator() {
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] kenhuuu commented on a diff in pull request #2087: Native transactions implementation
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#discussion_r1269948156
##########
tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransactionGraphTest.java:
##########
@@ -0,0 +1,722 @@
+/*
+ * 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.tinkergraph.structure;
+
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.util.TransactionException;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
+
+public class TinkerTransactionGraphTest {
Review Comment:
Performance and stress tests that were run against TinkerTransactionGraph can be found [here](https://github.com/Bit-Quill/tinkerpop/blob/ken/txntester/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/Tester.java). In particular, the functional testing portion of that Tester was used to discover multithreaded issues caused by running many transactions on the same Element.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] xiazcy commented on pull request #2087: Native transactions implementation
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #2087:
URL: https://github.com/apache/tinkerpop/pull/2087#issuecomment-1646282241
LGTM, VOTE +1. Thanks Valentyn!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org