[
https://issues.apache.org/jira/browse/TINKERPOP3-885?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14966768#comment-14966768
]
ASF GitHub Bot commented on TINKERPOP3-885:
-------------------------------------------
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/113#issuecomment-149887110
A few items - and sorry i didn't provide more information in the JIRA
ticket on this aspect, but it didn't become apparent to me until I looked at
your code....
You implemented the change in the `AbstractTransaction` which was good and
will work, but note that there are two extensions to that class that providers
can use: `AbstractThreadLocalTransaction` and `AbstractThreadedTransaction`.
You should move the `ThreadLocal` member variables you have to
`AbstractThreadLocalTransaction` as those pertain to that implementation. You
should move the "old" `readWriteConsumer` and `closeConsumer` to
`AbstractThreadedTransaction`. To make those work with existing usage in
`AbstractTransaction` you'll need abstract methods on `AbstractTransaction`
like `doReadWrite(Transaction)` and `doClose(Transaction)`. Then in the
respective sub-classes you implement those and with your implementation
specific member variables. Not a big change - just need to move some stuff
around - Does that make sense?
As for testing, your integration test covers this change from the top down
(plus you obviously have your driver tests) but I think it would be good to
include an explicit test in `TransactionTest`:
https://github.com/apache/incubator-tinkerpop/blob/master/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/TransactionTest.java
This way we enforce behavior on all Graph System providers that support
transactions. There is a chance that they will not extend
`AbstractTransaction` and use their own, so we'd want to ensure that this
functionality behaves no matter what they are trying to do or else folks will
try to use their `Graph` implementations in Gremlin Server and they could fail.
You would run tests against neo4j obviously...if you didn't know you need
to explicitly do:
```text
mvn clean install -DincludeNeo4j
```
see the CONTRIBUTING doc for info on how to just run the `TransactionTest`.
I think you would minimally want a test or two that has two threads going
where they each have their own independent settings for `readWriteConsumer` and
`closeConsumer`. Then you would want one where that ensured it was the same
setting across threads - make sure that one has this annotation:
```java
@FeatureRequirement(featureClass = Graph.Features.GraphFeatures.class,
feature = Graph.Features.GraphFeatures.FEATURE_THREADED_TRANSACTIONS)
```
Please yell if you need help sorting out what I wrote here.
> Change Transaction.onReadWrite() to be a ThreadLocal setting
> ------------------------------------------------------------
>
> Key: TINKERPOP3-885
> URL: https://issues.apache.org/jira/browse/TINKERPOP3-885
> Project: TinkerPop 3
> Issue Type: Improvement
> Components: server, structure
> Affects Versions: 3.0.1-incubating
> Reporter: Dylan Millikin
> Assignee: Dylan Millikin
> Labels: breaking
> Fix For: 3.1.0-incubating
>
>
> The issue is as follows:
> Because Transaction consumers are global for a graph. A
> {{SessionOpProcessor}} request will change the Transaction read write
> behavior to {{MANUAL}} across all concurrent/future requests.
> This will make other requests fail (ones expecting {{AUTO}}).
> This has been discussed here :
> http://mail-archives.apache.org/mod_mbox/incubator-tinkerpop-dev/201510.mbox/browser
> The solution would be to make Transaction consumers ThreadLocal to keep the
> state local to the requests.
> This is a breaking change.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
