Qpid
  1. Qpid
  2. QPID-3017

Add unit tests and improve error handling for classes within org.apache.qpid.server.txn

    Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.6, 0.7, 0.8
    • Fix Version/s: 0.9
    • Component/s: Java Broker
    • Labels:
      None
    • Environment:

      N/A

      Description

      The following new transaction classes:

      org.apache.qpid.server.txn.LocalTransaction
      org.apache.qpid.server.txn.AutoCommitTransaction
      org.apache.qpid.server.txn.ServerTransaction

      added for 0-10 currently have no unit tests and there are several TODOs flagged in the error handling logic.

        Activity

        Hide
        Robbie Gemmell added a comment -

        Patches applied.

        Show
        Robbie Gemmell added a comment - Patches applied.
        Hide
        Keith Wall added a comment -

        Patch uploaded to address comments from Robbie Gemmell.

        Show
        Keith Wall added a comment - Patch uploaded to address comments from Robbie Gemmell.
        Hide
        Robbie Gemmell added a comment -

        Hi Keith,

        Thanks for this, I had a look over the patches and they seem good. There are some small updates I'd suggest:

        • In AutoCommittransaction, one of the enqueue methods logs that it is dequeing instead of enqueing.
        • I would ensure the log messages convey that it is a store/transaction log dequeue being performed, as that logging isn't output for transient messages which isn't clear from the text.
        • The patches introduced whitespace after the penultimate closing brace in LocalTransaction.beginTranIfNecessary()
        • AMQException is now being caught in LocalTransaction.commit() instead of Exception. Given that any type of exception really indicates a need to clean up the transaction, I think it should still be catching Exception there.

        An additional patch just for updates to the originals is fine, easier even as it makes the changes obvious and I'll just squash them together at commit time.

        Robbie

        Show
        Robbie Gemmell added a comment - Hi Keith, Thanks for this, I had a look over the patches and they seem good. There are some small updates I'd suggest: In AutoCommittransaction, one of the enqueue methods logs that it is dequeing instead of enqueing. I would ensure the log messages convey that it is a store/transaction log dequeue being performed, as that logging isn't output for transient messages which isn't clear from the text. The patches introduced whitespace after the penultimate closing brace in LocalTransaction.beginTranIfNecessary() AMQException is now being caught in LocalTransaction.commit() instead of Exception. Given that any type of exception really indicates a need to clean up the transaction, I think it should still be catching Exception there. An additional patch just for updates to the originals is fine, easier even as it makes the changes obvious and I'll just squash them together at commit time. Robbie
        Hide
        Keith Wall added a comment -

        I've attached a patch to address this task.

        It covers:

        1) Unit tests:
        2) TODOs/javadoc
        3) Refactored method name ServerTransaction.addPostCommitAction to addPostTransactionAction to more accurately convey the purpose.
        4) Error handling:
        4a) LocalTransaction.java. There was a possibility of NullPointerException in method tidyUpOnError() if _transaction was ever null. Code now guards against this possibility.
        4b) LocalTransaction.java: Methods dequeue() and enqueue() ignored the Action (provided by the caller as formal parameter postTransactionAction) if an Exception was thrown by the TransactionLog methds. Code now ensures postTransactionAction is addedto the list _postTransactionAction in all cases.
        4c) AutoCommitTransaction.java: There was a possibility of a transaction being left in an open state. Code now guards against this possibility by aborting any such transaction.

        Show
        Keith Wall added a comment - I've attached a patch to address this task. It covers: 1) Unit tests: 2) TODOs/javadoc 3) Refactored method name ServerTransaction.addPostCommitAction to addPostTransactionAction to more accurately convey the purpose. 4) Error handling: 4a) LocalTransaction.java. There was a possibility of NullPointerException in method tidyUpOnError() if _transaction was ever null. Code now guards against this possibility. 4b) LocalTransaction.java: Methods dequeue() and enqueue() ignored the Action (provided by the caller as formal parameter postTransactionAction) if an Exception was thrown by the TransactionLog methds. Code now ensures postTransactionAction is addedto the list _postTransactionAction in all cases. 4c) AutoCommitTransaction.java: There was a possibility of a transaction being left in an open state. Code now guards against this possibility by aborting any such transaction.

          People

          • Assignee:
            Robbie Gemmell
            Reporter:
            Keith Wall
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development