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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
