Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-885

Change Transaction.onReadWrite() to be a ThreadLocal setting

    Details

      Description

      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.

        Activity

        Hide
        spmallette stephen mallette added a comment -

        Assigned to Dylan Millikin and adjusted title a bit.

        Show
        spmallette stephen mallette added a comment - Assigned to Dylan Millikin and adjusted title a bit.
        Hide
        spmallette stephen mallette added a comment -

        Is there much to do with Gremlin Server here? seems like all you need to do is make the change to ThreadLocal in Transaction for onReadWrite() - no?

        Show
        spmallette stephen mallette added a comment - Is there much to do with Gremlin Server here? seems like all you need to do is make the change to ThreadLocal in Transaction for onReadWrite() - no?
        Hide
        dmill Dylan Millikin added a comment -

        Yes this fixes the test. I've been holding out because I wanted to run the full test suit before making a PR. Just to make sure we aren't breaking something.

        I also realized there was a bit of a misunderstanding in that I never actually manually changed the transaction management. So the bug I had and reported the other day wasn't due to that. But I'll take a closer look at the problem I was experiencing and we can handle that separately.

        Show
        dmill Dylan Millikin added a comment - Yes this fixes the test. I've been holding out because I wanted to run the full test suit before making a PR. Just to make sure we aren't breaking something. I also realized there was a bit of a misunderstanding in that I never actually manually changed the transaction management. So the bug I had and reported the other day wasn't due to that. But I'll take a closer look at the problem I was experiencing and we can handle that separately.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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 <dylan.millikin@brightzone.fr>
        Date: 2015-10-20T12:29:44Z

        Made Transaction.onReadWrite() a ThreadLocal setting


        Show
        githubbot ASF GitHub Bot added a comment - 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 <dylan.millikin@brightzone.fr> Date: 2015-10-20T12:29:44Z Made Transaction.onReadWrite() a ThreadLocal setting
        Hide
        githubbot ASF GitHub Bot added a comment -

        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).

        Show
        githubbot ASF GitHub Bot added a comment - 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).
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user mhfrantz commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/113#issuecomment-149602003

        VOTE +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user mhfrantz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/113#issuecomment-149602003 VOTE +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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

        Show
        githubbot ASF GitHub Bot added a comment - 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
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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!!

        Show
        githubbot ASF GitHub Bot added a comment - 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!!
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user spmallette opened a pull request:

        https://github.com/apache/incubator-tinkerpop/pull/140

        TINKERPOP3-885 Revision of #113

        this is a revision of the work done here: #113 - the other had two +1s already but I thought that given the added changes it should be reviewed yet again. this commit - a806c7ec0a4fc5baaa1d684a647d0e5084f9c4cb - has the description of the changes since #113

        you can test with:

        ```text
        mvn clean install -DincludeNeo4j
        ```

        Note that I tested against Titan as well to validate the changes so that should give us added confidence that this change will work for everyone.

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP3-885

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/incubator-tinkerpop/pull/140.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 #140


        commit 8ea6aa25e8fb88f64c8b43572eb4e518233db4b1
        Author: Dylan Millikin <dylan.millikin@brightzone.fr>
        Date: 2015-10-20T12:29:44Z

        Made Transaction.onReadWrite() a ThreadLocal setting

        commit 5b0dd760a7d1c8609ffda2a415c18d541b2e51ff
        Author: Dylan Millikin <dylan.millikin@brightzone.fr>
        Date: 2015-10-21T12:31:16Z

        Merge branch 'master' into TINKERPOP-886

        commit 285bd28b523da9eacb8e84f270a0ebf906f6ca91
        Author: Dylan Millikin <dylan.millikin@brightzone.fr>
        Date: 2015-10-21T12:34:43Z

        removed typos

        commit 35e97f08e4359c0ae09f431c7680f8c13567549b
        Author: Dylan Millikin <dylan.millikin@brightzone.fr>
        Date: 2015-10-21T12:37:19Z

        moved Driver implementor section down in it's correct position

        commit 9881e6ecb651b3300dea1ad887d22ce1f4763b51
        Author: Stephen Mallette <spmva@genoprime.com>
        Date: 2015-10-21T13:03:35Z

        Merge remote-tracking branch 'origin/master' into TINKERPOP3-885

        commit 3938caed7fd0eb62453a2a2deb716e283c4b79a7
        Author: Dylan Millikin <dylan.millikin@brightzone.fr>
        Date: 2015-10-25T12:45:35Z

        moved logic to AbstractThreadLocalTransaction and added a couple of tests

        commit 46f821a846abc4fceb776f11c3b25888f01d60ef
        Author: Dylan Millikin <dylan.millikin@brightzone.fr>
        Date: 2015-10-25T12:45:35Z

        moved logic to AbstractThreadLocalTransaction and added a couple of tests

        commit 956d34de836693ea0bdd552ce434493ed7d1b14c
        Author: Dylan Millikin <dylan.millikin@brightzone.fr>
        Date: 2015-11-06T19:40:23Z

        Merge branch 'TP-885' of https://github.com/PommeVerte/incubator-tinkerpop into TP-885

        Conflicts:
        gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/TransactionTest.java

        commit 7ce6784466e8714edac465f2bbfee6dda5c019dd
        Author: Dylan Millikin <dylan.millikin@brightzone.fr>
        Date: 2015-11-06T21:40:57Z

        Merge remote-tracking branch 'tinkerpop/master' into TP-885

        Conflicts:
        docs/src/upgrade-release-3.1.x-incubating.asciidoc

        commit 789d6f70c7a76f2e6fb935d3dc1a659b8c0c871b
        Author: Dylan Millikin <dylan.millikin@gmail.com>
        Date: 2015-11-06T21:45:40Z

        Merge pull request #1 from PommeVerte/TP-885

        moved logic to AbstractThreadLocalTransaction and added a couple of tests

        commit 1aa1b5ac739a35ab1872ee5602a55c33cf035f29
        Author: Stephen Mallette <spmva@genoprime.com>
        Date: 2015-11-06T23:55:42Z

        Merge branch 'TINKERPOP-885' of https://github.com/PommeVerte/incubator-tinkerpop into TINKERPOP3-885

        commit a806c7ec0a4fc5baaa1d684a647d0e5084f9c4cb
        Author: Stephen Mallette <spmva@genoprime.com>
        Date: 2015-11-07T14:08:12Z

        TINKERPOP3-885 Adjustments to work from @PommeVerte from his PR at #113

        Removed two tests that didn't really test threaded transactions properly. Removed the static modifier on close/readWrite member variables which would cause a problem if the same thread was working with multiple graph instances. Removed setReadWrite and setClose from AbstractTransaction as they were redundnant to just onReadWrite and onClose (they offered no functionality in and of themselves). Altered the nature of AbstractThreadedTransaction a bit as threaded transactions are really "manual" by virtue of their creation. The transaction is opened when you createThreadedTransaction() and really shouldn't be re-used after close. An new one should be created. We don't have those semantics enforced now, but that's typically how this feature has been used in the past.

        commit 7ecd3538eaa2ab3581200e74f71ee1e1e45a4ea7
        Author: Stephen Mallette <spmva@genoprime.com>
        Date: 2015-11-07T14:29:07Z

        Updates to upgrade docs.

        commit 02623c7f59df26f18c4bb4acb859b55894b5ab3f
        Author: Stephen Mallette <spmva@genoprime.com>
        Date: 2015-11-07T14:30:09Z

        Update changelog.

        commit 6b532f78900f631b9d71d37c1b8293ec8adab71d
        Author: Stephen Mallette <spmva@genoprime.com>
        Date: 2015-11-07T14:32:34Z

        Update reference docs to denote the ThreadLocal nature of the transaction settings.


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/140 TINKERPOP3-885 Revision of #113 this is a revision of the work done here: #113 - the other had two +1s already but I thought that given the added changes it should be reviewed yet again. this commit - a806c7ec0a4fc5baaa1d684a647d0e5084f9c4cb - has the description of the changes since #113 you can test with: ```text mvn clean install -DincludeNeo4j ``` Note that I tested against Titan as well to validate the changes so that should give us added confidence that this change will work for everyone. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP3-885 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/140.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 #140 commit 8ea6aa25e8fb88f64c8b43572eb4e518233db4b1 Author: Dylan Millikin <dylan.millikin@brightzone.fr> Date: 2015-10-20T12:29:44Z Made Transaction.onReadWrite() a ThreadLocal setting commit 5b0dd760a7d1c8609ffda2a415c18d541b2e51ff Author: Dylan Millikin <dylan.millikin@brightzone.fr> Date: 2015-10-21T12:31:16Z Merge branch 'master' into TINKERPOP-886 commit 285bd28b523da9eacb8e84f270a0ebf906f6ca91 Author: Dylan Millikin <dylan.millikin@brightzone.fr> Date: 2015-10-21T12:34:43Z removed typos commit 35e97f08e4359c0ae09f431c7680f8c13567549b Author: Dylan Millikin <dylan.millikin@brightzone.fr> Date: 2015-10-21T12:37:19Z moved Driver implementor section down in it's correct position commit 9881e6ecb651b3300dea1ad887d22ce1f4763b51 Author: Stephen Mallette <spmva@genoprime.com> Date: 2015-10-21T13:03:35Z Merge remote-tracking branch 'origin/master' into TINKERPOP3-885 commit 3938caed7fd0eb62453a2a2deb716e283c4b79a7 Author: Dylan Millikin <dylan.millikin@brightzone.fr> Date: 2015-10-25T12:45:35Z moved logic to AbstractThreadLocalTransaction and added a couple of tests commit 46f821a846abc4fceb776f11c3b25888f01d60ef Author: Dylan Millikin <dylan.millikin@brightzone.fr> Date: 2015-10-25T12:45:35Z moved logic to AbstractThreadLocalTransaction and added a couple of tests commit 956d34de836693ea0bdd552ce434493ed7d1b14c Author: Dylan Millikin <dylan.millikin@brightzone.fr> Date: 2015-11-06T19:40:23Z Merge branch 'TP-885' of https://github.com/PommeVerte/incubator-tinkerpop into TP-885 Conflicts: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/TransactionTest.java commit 7ce6784466e8714edac465f2bbfee6dda5c019dd Author: Dylan Millikin <dylan.millikin@brightzone.fr> Date: 2015-11-06T21:40:57Z Merge remote-tracking branch 'tinkerpop/master' into TP-885 Conflicts: docs/src/upgrade-release-3.1.x-incubating.asciidoc commit 789d6f70c7a76f2e6fb935d3dc1a659b8c0c871b Author: Dylan Millikin <dylan.millikin@gmail.com> Date: 2015-11-06T21:45:40Z Merge pull request #1 from PommeVerte/TP-885 moved logic to AbstractThreadLocalTransaction and added a couple of tests commit 1aa1b5ac739a35ab1872ee5602a55c33cf035f29 Author: Stephen Mallette <spmva@genoprime.com> Date: 2015-11-06T23:55:42Z Merge branch ' TINKERPOP-885 ' of https://github.com/PommeVerte/incubator-tinkerpop into TINKERPOP3-885 commit a806c7ec0a4fc5baaa1d684a647d0e5084f9c4cb Author: Stephen Mallette <spmva@genoprime.com> Date: 2015-11-07T14:08:12Z TINKERPOP3-885 Adjustments to work from @PommeVerte from his PR at #113 Removed two tests that didn't really test threaded transactions properly. Removed the static modifier on close/readWrite member variables which would cause a problem if the same thread was working with multiple graph instances. Removed setReadWrite and setClose from AbstractTransaction as they were redundnant to just onReadWrite and onClose (they offered no functionality in and of themselves). Altered the nature of AbstractThreadedTransaction a bit as threaded transactions are really "manual" by virtue of their creation. The transaction is opened when you createThreadedTransaction() and really shouldn't be re-used after close. An new one should be created. We don't have those semantics enforced now, but that's typically how this feature has been used in the past. commit 7ecd3538eaa2ab3581200e74f71ee1e1e45a4ea7 Author: Stephen Mallette <spmva@genoprime.com> Date: 2015-11-07T14:29:07Z Updates to upgrade docs. commit 02623c7f59df26f18c4bb4acb859b55894b5ab3f Author: Stephen Mallette <spmva@genoprime.com> Date: 2015-11-07T14:30:09Z Update changelog. commit 6b532f78900f631b9d71d37c1b8293ec8adab71d Author: Stephen Mallette <spmva@genoprime.com> Date: 2015-11-07T14:32:34Z Update reference docs to denote the ThreadLocal nature of the transaction settings.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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?

        Show
        githubbot ASF GitHub Bot added a comment - 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?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user PommeVerte closed the pull request at:

        https://github.com/apache/incubator-tinkerpop/pull/113

        Show
        githubbot ASF GitHub Bot added a comment - Github user PommeVerte closed the pull request at: https://github.com/apache/incubator-tinkerpop/pull/113
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user dkuppitz commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/140#issuecomment-155049421

        Nice and solid code.

        VOTE: +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/140#issuecomment-155049421 Nice and solid code. VOTE: +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user PommeVerte commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/140#issuecomment-155050162

        Like the changes. This looks nice.
        Tests pass.
        Definite +1 from me.

        Show
        githubbot ASF GitHub Bot added a comment - Github user PommeVerte commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/140#issuecomment-155050162 Like the changes. This looks nice. Tests pass. Definite +1 from me.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/incubator-tinkerpop/pull/140

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/incubator-tinkerpop/pull/140

          People

          • Assignee:
            dmill Dylan Millikin
            Reporter:
            dmill Dylan Millikin
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development