Cole-Greer commented on code in PR #2395: URL: https://github.com/apache/tinkerpop/pull/2395#discussion_r1425691408
########## docs/src/dev/future/proposal-multi-threaded-transaction-5.asciidoc: ########## @@ -0,0 +1,146 @@ +//// +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. +//// +image::apache-tinkerpop-logo.png[width=500,link="https://tinkerpop.apache.org";] + +*x.y.z - Proposal 4* + +Proposal for adding support for tx which are not thread bound. + +=== Introduction +The current implementation of transactions and sessions uses approach with thread local variables that requires complex implementation and LifeCycle management. Using non-threaded sessions can improve performance in some scenarios. + +=== Motivation +* Simlify GremlinExecutor. Complex LifeCycle control is now used. +* Easier integration with HTTP because transactions can be used without being tied to a connection. + +=== Assumptions + +* Need to reuse existing code as much as possible. +* Users should be able to work both types of transactions, thread-bound and new non-thread bound. Perhaps after performance testing only one type will be left. +* Transaction implementation should not have additional external dependencies. +* Existing drivers should work without changes. Review Comment: In my opinion, existing driver compatibility is a strong plus, but not a strict requirement if these changes are targeted to TP 4. I would view this as a soft requirement that could be dropped with proper justification. ########## docs/src/dev/future/proposal-multi-threaded-transaction-5.asciidoc: ########## @@ -0,0 +1,146 @@ +//// +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. +//// +image::apache-tinkerpop-logo.png[width=500,link="https://tinkerpop.apache.org";] + +*x.y.z - Proposal 4* + +Proposal for adding support for tx which are not thread bound. + +=== Introduction +The current implementation of transactions and sessions uses approach with thread local variables that requires complex implementation and LifeCycle management. Using non-threaded sessions can improve performance in some scenarios. + +=== Motivation +* Simlify GremlinExecutor. Complex LifeCycle control is now used. Review Comment: ```suggestion * Simplify GremlinExecutor. Complex LifeCycle control is now used. ``` ########## docs/src/dev/future/proposal-multi-threaded-transaction-5.asciidoc: ########## @@ -0,0 +1,146 @@ +//// +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. +//// +image::apache-tinkerpop-logo.png[width=500,link="https://tinkerpop.apache.org";] + +*x.y.z - Proposal 4* + +Proposal for adding support for tx which are not thread bound. + +=== Introduction +The current implementation of transactions and sessions uses approach with thread local variables that requires complex implementation and LifeCycle management. Using non-threaded sessions can improve performance in some scenarios. + +=== Motivation +* Simlify GremlinExecutor. Complex LifeCycle control is now used. +* Easier integration with HTTP because transactions can be used without being tied to a connection. + +=== Assumptions + +* Need to reuse existing code as much as possible. +* Users should be able to work both types of transactions, thread-bound and new non-thread bound. Perhaps after performance testing only one type will be left. +* Transaction implementation should not have additional external dependencies. +* Existing drivers should work without changes. + + +=== Specifications +==== Design Overview +The main components are Graph with committed data and Graph with tx specific (modified or cached for consistent read) data. +At the stage of resolving bindings for Graph and GraphTraversalSource need to create an instance that will be specific only for the current session/tx and use it until the end of the operation ending with commit/rollback. Now bindings are stored in different places, for example GremlinScriptEngineManager.globalScope and GraphManager.getTraversalSource/getGraph, also need to add a used Graph and GraphTraversalSource to Session. + + +==== Overview of changes to code base +===== TinkerTransactionGraph2 (suggest a better name) +Storage for committed data. Should not be used directly for graph operations. +[code] +---- +class TinkerTransactionGraph2 implements Graph { + private final Map<Object, TinkerElementLockContainer<TinkerVertex>> vertices = new ConcurrentHashMap<>(); + + // I haven't decided which option is better yet + public Transaction tx() { throw new Exception("use TinkerGraphTx instead"); } + public Transaction tx() { return new TinkerGraphTx(this); } +} +---- + +===== TinkerElementLockContainer +Storage for Element and lock for concurrent modification. +[code] +---- +class TinkerElementLockContainer<T extends TinkerElement> { + T element; + ReentrantLock lock = new ReentrantLock(); +} +---- + +===== TinkerGraphTx (suggest a better name) +Graph for use in single transaction +[code] +---- +class TinkerGraphTx implements Graph, Transaction { + private final Map<Object, TinkerElementTxContainer<TinkerVertex>> changedVertices = new ConcurrentHashMap<>(); + private final TinkerTransactionGraph2 commitedGraph; + + public Transaction tx() { return this; } + + public Vertex addVertex(final Object... keyValues) { + // create and add vertex to changedVertices + } + + public Vertex vertex(final Object vertexId) { + // search for vertex in changedVertices, if not found than in commitedGraph + } + + protected void doCommit() throws TransactionException { + // propagate changes to commitedGraph + } +} +---- + +===== TinkerElementTxContainer (suggest a better name) +Storage for element used in transaction. +Can be unmodified (cached for consistent read) or created/modified in current tx. +Unchanged element is simply a reference to the element that was there at the time of first reading. When a change +is made, a clone is created and further work is done with it. +[code] +---- +final class TinkerElementTxContainer<T extends TinkerElement> { + T element; + boolean isDeleted; + boolean isModified; +} +---- + + +==== CRUD operations without transaction +1. Create TinkerGraphTx and GraphTraversalSource from it. +2. Execute traversal +3. Commit or rollback on error. + + +==== CRUD operations with transaction +===== Create +Add new TinkerElementTxContainer to `vertices` and `edges` in TinkerGraphTx. + +==== Read +Read values from `vertices` TinkerElementTxContainer in TinkerGraphTx. +If marked as deleted than return null; +If not found try TinkerTransactionGraph2.vertices. + +===== Update +Add or update if exist corresponding TinkerElementTxContainer in `vertices`. Review Comment: Would any CRUD operation on a property essentially be processed as an Update to its parent/grandparent vertex/edge? ########## docs/src/dev/future/proposal-multi-threaded-transaction-5.asciidoc: ########## @@ -0,0 +1,146 @@ +//// +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. +//// +image::apache-tinkerpop-logo.png[width=500,link="https://tinkerpop.apache.org";] + +*x.y.z - Proposal 4* + +Proposal for adding support for tx which are not thread bound. + +=== Introduction +The current implementation of transactions and sessions uses approach with thread local variables that requires complex implementation and LifeCycle management. Using non-threaded sessions can improve performance in some scenarios. + +=== Motivation +* Simlify GremlinExecutor. Complex LifeCycle control is now used. +* Easier integration with HTTP because transactions can be used without being tied to a connection. + +=== Assumptions + +* Need to reuse existing code as much as possible. +* Users should be able to work both types of transactions, thread-bound and new non-thread bound. Perhaps after performance testing only one type will be left. +* Transaction implementation should not have additional external dependencies. +* Existing drivers should work without changes. + + +=== Specifications +==== Design Overview +The main components are Graph with committed data and Graph with tx specific (modified or cached for consistent read) data. +At the stage of resolving bindings for Graph and GraphTraversalSource need to create an instance that will be specific only for the current session/tx and use it until the end of the operation ending with commit/rollback. Now bindings are stored in different places, for example GremlinScriptEngineManager.globalScope and GraphManager.getTraversalSource/getGraph, also need to add a used Graph and GraphTraversalSource to Session. + + +==== Overview of changes to code base +===== TinkerTransactionGraph2 (suggest a better name) +Storage for committed data. Should not be used directly for graph operations. +[code] +---- +class TinkerTransactionGraph2 implements Graph { + private final Map<Object, TinkerElementLockContainer<TinkerVertex>> vertices = new ConcurrentHashMap<>(); + + // I haven't decided which option is better yet + public Transaction tx() { throw new Exception("use TinkerGraphTx instead"); } + public Transaction tx() { return new TinkerGraphTx(this); } +} +---- + +===== TinkerElementLockContainer +Storage for Element and lock for concurrent modification. +[code] +---- +class TinkerElementLockContainer<T extends TinkerElement> { + T element; + ReentrantLock lock = new ReentrantLock(); +} +---- + +===== TinkerGraphTx (suggest a better name) +Graph for use in single transaction +[code] +---- +class TinkerGraphTx implements Graph, Transaction { + private final Map<Object, TinkerElementTxContainer<TinkerVertex>> changedVertices = new ConcurrentHashMap<>(); + private final TinkerTransactionGraph2 commitedGraph; + + public Transaction tx() { return this; } + + public Vertex addVertex(final Object... keyValues) { + // create and add vertex to changedVertices + } + + public Vertex vertex(final Object vertexId) { + // search for vertex in changedVertices, if not found than in commitedGraph + } + + protected void doCommit() throws TransactionException { + // propagate changes to commitedGraph + } +} +---- + +===== TinkerElementTxContainer (suggest a better name) +Storage for element used in transaction. +Can be unmodified (cached for consistent read) or created/modified in current tx. +Unchanged element is simply a reference to the element that was there at the time of first reading. When a change +is made, a clone is created and further work is done with it. +[code] +---- +final class TinkerElementTxContainer<T extends TinkerElement> { + T element; + boolean isDeleted; + boolean isModified; +} +---- + + +==== CRUD operations without transaction +1. Create TinkerGraphTx and GraphTraversalSource from it. +2. Execute traversal +3. Commit or rollback on error. + + +==== CRUD operations with transaction +===== Create +Add new TinkerElementTxContainer to `vertices` and `edges` in TinkerGraphTx. + +==== Read +Read values from `vertices` TinkerElementTxContainer in TinkerGraphTx. +If marked as deleted than return null; +If not found try TinkerTransactionGraph2.vertices. Review Comment: Would we want to add the following to ensure consistent reads? If found in `TinkerTransactionGraph2.vertices`, copy to `vertices` TinkerElementTxContainer in TinkerGraphTx? ########## docs/src/dev/future/proposal-multi-threaded-transaction-5.asciidoc: ########## @@ -0,0 +1,146 @@ +//// +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. +//// +image::apache-tinkerpop-logo.png[width=500,link="https://tinkerpop.apache.org";] + +*x.y.z - Proposal 4* + +Proposal for adding support for tx which are not thread bound. + +=== Introduction +The current implementation of transactions and sessions uses approach with thread local variables that requires complex implementation and LifeCycle management. Using non-threaded sessions can improve performance in some scenarios. + +=== Motivation +* Simlify GremlinExecutor. Complex LifeCycle control is now used. +* Easier integration with HTTP because transactions can be used without being tied to a connection. + +=== Assumptions + +* Need to reuse existing code as much as possible. +* Users should be able to work both types of transactions, thread-bound and new non-thread bound. Perhaps after performance testing only one type will be left. Review Comment: Is this statement specifically about the transaction model in TinkerGraph or for all providers who support transactions? I agree that TinkerGraph should eventually converge on a single model, however I imagine there would be some providers who would like to retain a thread-bound model. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
