You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by PommeVerte <gi...@git.apache.org> on 2015/10/20 15:09:30 UTC
[GitHub] incubator-tinkerpop pull request: TINKERPOP3-885 Made Transaction....
GitHub user PommeVerte opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/113
TINKERPOP3-885 Made Transaction.onReadWrite() a ThreadLocal setting
JIRA issue : https://issues.apache.org/jira/browse/TINKERPOP3-885
Both transaction consumers have been made `ThreadLocal` settings.
Since this is a breaking change I've made some additions to the upgrade document. It's worth noting that I didn't put anything in there for graph providers as I wasn't sure what to put. Feel free to change/remove/add to this. (or tell me and I'll be more than happy to add to it myself)
I've also only added the initial failing test we had found. If you can think of anymore let me know.
PS: I've tried testing the full suit but for some reason my computer isn't cooperating and it's just taking too much time (and ressources) so I had to stop it. If any of you could give it a go I'd be grateful. Tests passed for gremlin-server, gremlin-core, gremlin-groovy(-test), tinkergraph-gremlin from what I've seen. Not sure about the others.
I'll try and get some stable testing plateform up sometime soon.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/PommeVerte/incubator-tinkerpop TINKERPOP-885
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/113.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #113
----
commit 8ea6aa25e8fb88f64c8b43572eb4e518233db4b1
Author: Dylan Millikin <dy...@brightzone.fr>
Date: 2015-10-20T12:29:44Z
Made Transaction.onReadWrite() a ThreadLocal setting
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP3-885 Made Transaction....
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/113#issuecomment-154728797
I've recast this work under this PR #140 - no need for any more votes on this one. @PommeVerte can you please close this one on your end?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP3-885 Made Transaction....
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/113#issuecomment-149860764
@PommeVerte can you please merge up master into this pr and resolve the conflicts in your fork? I can continue review once you do that. thanks.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP3-885 Made Transaction....
Posted by PommeVerte <gi...@git.apache.org>.
Github user PommeVerte closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/113
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP3-885 Made Transaction....
Posted by mhfrantz <gi...@git.apache.org>.
Github user mhfrantz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/113#issuecomment-149602003
VOTE +1
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP3-885 Made Transaction....
Posted by PommeVerte <gi...@git.apache.org>.
Github user PommeVerte commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/113#issuecomment-149878701
Done, it was a conflict with the upgrade documentation
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP3-885 Made Transaction....
Posted by PommeVerte <gi...@git.apache.org>.
Github user PommeVerte commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/113#issuecomment-149890177
Ok that all makes perfect sense (I should have picked up on the AbstractThreadLocalTransaction), I'll go ahead with those changes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP3-885 Made Transaction....
Posted by PommeVerte <gi...@git.apache.org>.
Github user PommeVerte commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/113#issuecomment-154552506
Ok sorry about the delay with this. It took me some time to wrap my head around the StandardStructureTestSuit and adding my crappy testing env to that suddenly made things way more complicated.
Anyways I've tested this with :
```
mvn clean install -DincludeNeo4j
```
And I've also run related integration tests (though not all as my poor ubuntu VM wouldn't manage.. I'm going to fix the builds on my mac this weekend to have a clean dev environment and avoid this in the future)
I haven't managed to run the tests against titan though (for the threaded transactions) If someone could give those a look it would perfect. Normally the tests should be properly written.
Thanks!!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP3-885 Made Transaction....
Posted by spmallette <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP3-885 Made Transaction....
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/113#issuecomment-149592080
From a purely aesthetic perspective, this code is clean and pretty.
VOTE +1 (binding).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---