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

Unify semantics of Transaction.close() in tests and documentation

    Details

      Description

      CLOSE_BEHAVIOR implementations only close
      the transaction of the current thread that closed the graph which potentially leaves open others. Consider methods to improve on this.

        Activity

        Hide
        spmallette stephen mallette added a comment -

        Given some discussion on the dev mailing list:

        http://mail-archives.apache.org/mod_mbox/incubator-tinkerpop-dev/201508.mbox/%3CCAA-H4380Hc9H8XU7hx%2BwsXRG_cBqLwYDuEk7HiOgwj-xe8CLkg%40mail.gmail.com%3E

        The general feeling was that we shouldn't deprecate Transaction.close(). I left CLOSE_BEHAVIOR in place as well. Other changes were left in place with respect to unifying semantics in tests/docs, thus this remains a potentially breaking change.

        Please see TINKERPOP3-805 for information on how this work might continue post 3.0.1.

        Show
        spmallette stephen mallette added a comment - Given some discussion on the dev mailing list: http://mail-archives.apache.org/mod_mbox/incubator-tinkerpop-dev/201508.mbox/%3CCAA-H4380Hc9H8XU7hx%2BwsXRG_cBqLwYDuEk7HiOgwj-xe8CLkg%40mail.gmail.com%3E The general feeling was that we shouldn't deprecate Transaction.close() . I left CLOSE_BEHAVIOR in place as well. Other changes were left in place with respect to unifying semantics in tests/docs, thus this remains a potentially breaking change. Please see TINKERPOP3-805 for information on how this work might continue post 3.0.1.
        Hide
        spmallette stephen mallette added a comment -

        I've moved forward with deprecating Transaction.close() and related CLOSE_BEHAVIOR as described above in the previous comment. That in and of itself does not represent a "breaking change", however, there were minor changes in relation to test semantics and test naming in TransactionTest

        • shouldCommitOnShutdownByDefault is now shouldCommitOnCloseByDefault
        • shouldRollbackOnShutdownWhenConfigured is now shouldRollbackOnCloseWhenConfigured

        As a result, implementers should be on the lookout for possible test failures in this area (which should be straightforward to fix).

        Show
        spmallette stephen mallette added a comment - I've moved forward with deprecating Transaction.close() and related CLOSE_BEHAVIOR as described above in the previous comment. That in and of itself does not represent a "breaking change", however, there were minor changes in relation to test semantics and test naming in TransactionTest shouldCommitOnShutdownByDefault is now shouldCommitOnCloseByDefault shouldRollbackOnShutdownWhenConfigured is now shouldRollbackOnCloseWhenConfigured As a result, implementers should be on the lookout for possible test failures in this area (which should be straightforward to fix).
        Hide
        spmallette stephen mallette added a comment -

        I didn't change much code here yet, but I think I straightened out some confusing semantics. CLOSE_BEHAVIOR had some mixed meaning in terms of documentation, usage and test cases. Sometimes it referred to how a Graph should behave on close and sometimes it referred to closing the "active" transaction.

        I think that the original intention for Transaction.close was to supply configurable behavior to Graph.close. I'm not sure I like that anymore for a couple reasons:

        1. It's too easy for a user to call graph.tx().close() thinking of it as closing a transaction which it is doing in a ThreadLocal way, but that wasn't the intended purpose.
        2. The CLOSE_BEHAVIOR isn't useful outside of the current thread, so calling Transaction.close from Graph.close doesn't really have the intended effect.

        So, thus far, I've cleaned up the docs in 3.0.1 to indicate that the Transaction.close method is to "close the current transaction". I've also altered the Gremlin Test Suite to stop enforcing CLOSE_BEHAVIOR from Graph.close. That change actually did something interesting in that I was able to remove the one OptOut from Neo4jGraph - all tests now pass.

        So that leaves us with clear semantics, but more change might still be in order. I'm of the opinion that we should deprecate Transaction.close and at some point remove Closeable as an interface it implements. I don't think we want to encourage this style:

        try (Transaction tx = graph.tx()) {
          ...
        }
        

        and then, why would anyone do:

        graph.addVertex()
        graph.tx().close()
        

        They would be better served to do:

        try {
          graph.addVertex()
          graph.tx().commit()
        } catch (Exception ex) {
          graph.tx().rollback()
        }
        
        Show
        spmallette stephen mallette added a comment - I didn't change much code here yet, but I think I straightened out some confusing semantics. CLOSE_BEHAVIOR had some mixed meaning in terms of documentation, usage and test cases. Sometimes it referred to how a Graph should behave on close and sometimes it referred to closing the "active" transaction. I think that the original intention for Transaction.close was to supply configurable behavior to Graph.close . I'm not sure I like that anymore for a couple reasons: 1. It's too easy for a user to call graph.tx().close() thinking of it as closing a transaction which it is doing in a ThreadLocal way, but that wasn't the intended purpose. 2. The CLOSE_BEHAVIOR isn't useful outside of the current thread, so calling Transaction.close from Graph.close doesn't really have the intended effect. So, thus far, I've cleaned up the docs in 3.0.1 to indicate that the Transaction.close method is to "close the current transaction". I've also altered the Gremlin Test Suite to stop enforcing CLOSE_BEHAVIOR from Graph.close . That change actually did something interesting in that I was able to remove the one OptOut from Neo4jGraph - all tests now pass. So that leaves us with clear semantics, but more change might still be in order. I'm of the opinion that we should deprecate Transaction.close and at some point remove Closeable as an interface it implements. I don't think we want to encourage this style: try (Transaction tx = graph.tx()) { ... } and then, why would anyone do: graph.addVertex() graph.tx().close() They would be better served to do: try { graph.addVertex() graph.tx().commit() } catch (Exception ex) { graph.tx().rollback() }

          People

          • Assignee:
            spmallette stephen mallette
            Reporter:
            spmallette stephen mallette
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development