Details

    • Type: Improvement Improvement
    • Status: Closed
    • 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

        Keith Wall created issue -
        Keith Wall made changes -
        Field Original Value New Value
        Summary Refactor TransactionTimeout and avoid sporadic test fails on slower CI hosts Refactor TransactionTimeout
        Keith Wall made changes -
        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.

        Another motivation for this change it that TransactionTimeoutTest is still sporadically failing on CI (probably contributing more than 50% of the fails we see on Jenkins) and creates much noise. This change will aim to resolve this problem at the same time.




        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.




        Keith Wall made changes -
        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?
        Keith Wall made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Keith Wall made changes -
        Assignee Keith Wall [ k-wall ] Philip Harvey [ philharveyonline ]
        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.
        Philip Harvey made changes -
        Assignee Philip Harvey [ philharveyonline ] Keith Wall [ k-wall ]
        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.
        Keith Wall made changes -
        Keith Wall made changes -
        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.
        Keith Wall made changes -
        Assignee Keith Wall [ k-wall ] Philip Harvey [ philharveyonline ]
        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.
        Philip Harvey made changes -
        Philip Harvey made changes -
        Assignee Philip Harvey [ philharveyonline ] Keith Wall [ k-wall ]
        Keith Wall made changes -
        Status In Progress [ 3 ] Ready To Review [ 10006 ]
        Keith Wall made changes -
        Status Ready To Review [ 10006 ] Resolved [ 5 ]
        Fix Version/s 0.21 [ 12323549 ]
        Resolution Fixed [ 1 ]
        Rob Godfrey made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open In Progress In Progress
        427d 14h 20m 1 Keith Wall 01/Jan/13 22:54
        In Progress In Progress Reviewable Reviewable
        5d 18h 42m 1 Keith Wall 07/Jan/13 17:37
        Reviewable Reviewable Resolved Resolved
        15s 1 Keith Wall 07/Jan/13 17:37
        Resolved Resolved Closed Closed
        765d 2h 28m 1 Rob Godfrey 11/Feb/15 20:05

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development