Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21
    • Component/s: Java Broker
    • Labels:
      None

      Description

      The Transaction Timeout feature of the Java Broker relies heavily on system tests. It would be better if its implementation were refactored to allow more of its functionality to be tested by unit tests and a reduced number of system tests.

        Activity

        Hide
        Keith Wall added a comment - - edited

        Proposed changes are as follows:

        1) Moved the duplicated transactionUpdateTime member from AMQChannel/ServerSession to ServerTransaction.
        a) LocalTransaction now maintains advances transactionUpdateTime on each enqueue/dequeue operation
        b) Other non-transactional ServerTransaction impls return transactionUpdateTime of 0 (as they already do for transactionStartTime).
        c) Changed LocalTransaction so that transaction start time is recorded on first enqueue or dequeue operation (rather than only
        first enqueue)
        2) Moved duplicated logic from AMQChannel/ServerSession#checkTransactionStatus to TransactionTimeoutHelper
        3) Make TransactionTimeoutTests use a durable queue so it is actually testing with store transactions.

        I plan to slim down TransactionTimeoutTests so we have fewer system tests, but this is not done yet.

        Phil, could I ask you to review the attached patch and comment please?

        Show
        Keith Wall added a comment - - edited Proposed changes are as follows: 1) Moved the duplicated transactionUpdateTime member from AMQChannel/ServerSession to ServerTransaction. a) LocalTransaction now maintains advances transactionUpdateTime on each enqueue/dequeue operation b) Other non-transactional ServerTransaction impls return transactionUpdateTime of 0 (as they already do for transactionStartTime). c) Changed LocalTransaction so that transaction start time is recorded on first enqueue or dequeue operation (rather than only first enqueue) 2) Moved duplicated logic from AMQChannel/ServerSession#checkTransactionStatus to TransactionTimeoutHelper 3) Make TransactionTimeoutTests use a durable queue so it is actually testing with store transactions. I plan to slim down TransactionTimeoutTests so we have fewer system tests, but this is not done yet. Phil, could I ask you to review the attached patch and comment please?
        Hide
        Philip Harvey added a comment -

        My initial thoughts are:

        • TransactionTimeoutHelper.check still has quite a lot of duplication, i.e. for the open and idle paths. Would the current refactoring be a good opportunity to resolve this duplication? If so, could we change:
          final long transactionStartTime = _transaction.getTransactionStartTime();
          final long transactionUpdateTime = _transaction.getTransactionUpdateTime();
          if (transactionUpdateTime > 0 && transactionStartTime > 0)
          {
              // do idle and open logic
          }
          

          to:

          final long transactionStartTime = _transaction.getTransactionStartTime();
          if (transactionUpdateTime > 0)
          {
              // do idle logic
          }
          final long transactionUpdateTime = _transaction.getTransactionUpdateTime();
          if(transactionStartTime > 0)
          {
              // do open logic
          }
          
        • The calls to logIfNecessary in the check(..) method have incorrect indentation
        • Do the getTransactionUpdateTime() implementations need an @Override annotation?
        Show
        Philip Harvey added a comment - My initial thoughts are: TransactionTimeoutHelper.check still has quite a lot of duplication, i.e. for the open and idle paths. Would the current refactoring be a good opportunity to resolve this duplication? If so, could we change: final long transactionStartTime = _transaction.getTransactionStartTime(); final long transactionUpdateTime = _transaction.getTransactionUpdateTime(); if (transactionUpdateTime > 0 && transactionStartTime > 0) { // do idle and open logic } to: final long transactionStartTime = _transaction.getTransactionStartTime(); if (transactionUpdateTime > 0) { // do idle logic } final long transactionUpdateTime = _transaction.getTransactionUpdateTime(); if(transactionStartTime > 0) { // do open logic } The calls to logIfNecessary in the check(..) method have incorrect indentation Do the getTransactionUpdateTime() implementations need an @Override annotation?
        Hide
        Philip Harvey added a comment -

        added initial review comments... let's discuss these before I do a detailed sweep of the other changes.

        Show
        Philip Harvey added a comment - added initial review comments... let's discuss these before I do a detailed sweep of the other changes.
        Hide
        Robbie Gemmell added a comment -

        One trivial thing I noted was the use of an AtomicLong for the update time tracking vs a volatile long for the start time tracking. It might be nicer if they were both the same, and since we don't use the CAS etc operations from the AtomicLong it seems like the volatile long is preferable.

        Show
        Robbie Gemmell added a comment - One trivial thing I noted was the use of an AtomicLong for the update time tracking vs a volatile long for the start time tracking. It might be nicer if they were both the same, and since we don't use the CAS etc operations from the AtomicLong it seems like the volatile long is preferable.
        Hide
        Keith Wall added a comment -

        Hi Phil, I've reattached a patch containing the changes we worked on together. This already addresses Robbie's earlier comment.

        Show
        Keith Wall added a comment - Hi Phil, I've reattached a patch containing the changes we worked on together. This already addresses Robbie's earlier comment.
        Hide
        Philip Harvey added a comment -

        I reviewed all the code changes and it all looks good to me. I'm attaching a modified patch that just adds some missing @Override annotations and fixes minor formatting.
        I think this patch can be committed.

        Show
        Philip Harvey added a comment - I reviewed all the code changes and it all looks good to me. I'm attaching a modified patch that just adds some missing @Override annotations and fixes minor formatting. I think this patch can be committed.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development