Qpid
  1. Qpid
  2. QPID-3867

AMQQueueMBean#clearQueue|moveMessages|copyMessages should be certain to rollback transactions in the event of exception

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.15
    • Fix Version/s: 0.15
    • Component/s: Java Broker
    • Labels:
      None

      Description

      AMQQueueMBean#clearQueue|moveMessages|copyMessages all leave open that possibility that a LocalTransaction goes uncommitted (or unrolledback) in the event of exception.

      This will lead to uncontrolled store growth when used with BDB as would block the action of the cleaner thread.

      A finally block should be used to ensure that transaction is rolled back on any exception.

        Activity

        Hide
        Keith Wall added a comment -

        Hi Robbie, can you review this commit please?

        Show
        Keith Wall added a comment - Hi Robbie, can you review this commit please?
        Hide
        Rob Godfrey added a comment -

        One small comment from me:

        Since we're changing the interface on AMQQueue anyway, I would suggest it would be nicer for the move/copy to take the destination queue as an AMQQueue rather than as a string which needs to be looked up. Obviously the methods still need to check that the passed queue != this, and that the queue is on the same vhost.

        Not a blocker by any means, rest of the patch looked good to me.

        Show
        Rob Godfrey added a comment - One small comment from me: Since we're changing the interface on AMQQueue anyway, I would suggest it would be nicer for the move/copy to take the destination queue as an AMQQueue rather than as a string which needs to be looked up. Obviously the methods still need to check that the passed queue != this, and that the queue is on the same vhost. Not a blocker by any means, rest of the patch looked good to me.
        Hide
        Robbie Gemmell added a comment -

        My only reservation is around the queue depth checking in the tests, I think its possible that will sporadically fail on certain slower CI instances due to the commit response going back before the in-memory enqueue is completed.

        Im not too fussed about the queue parameter. The problem with doing that would be that the MBean would then need to be the one doing the lookup so that it could provide the AMQQueue object, which isnt really any nicer since it would be good if the management layer was as dumb-as-a-brick where possible and AMQQueueMBean is already an example of where its not.

        Show
        Robbie Gemmell added a comment - My only reservation is around the queue depth checking in the tests, I think its possible that will sporadically fail on certain slower CI instances due to the commit response going back before the in-memory enqueue is completed. Im not too fussed about the queue parameter. The problem with doing that would be that the MBean would then need to be the one doing the lookup so that it could provide the AMQQueue object, which isnt really any nicer since it would be good if the management layer was as dumb-as-a-brick where possible and AMQQueueMBean is already an example of where its not.
        Hide
        Rob Godfrey added a comment -

        I guess my issue is that the API on AMQQueue feels fairly "unnatural"... it's clearly bolted on to support a particular function of the MBean rather than naturally being something the Queue would expose, and we should probably be doing our best to simplify AMQQueue.

        In that sense I feel like actually the functionality should be moved into a third-party class (including the management of the transaction). The API on AMQQueue should potentially just be the ability to acquire a set of messages matching a filter. In that sense I think the change to make the subject queue manage the transaction is suspect.

        This is possibly worth a separate JIRA though.

        Show
        Rob Godfrey added a comment - I guess my issue is that the API on AMQQueue feels fairly "unnatural"... it's clearly bolted on to support a particular function of the MBean rather than naturally being something the Queue would expose, and we should probably be doing our best to simplify AMQQueue. In that sense I feel like actually the functionality should be moved into a third-party class (including the management of the transaction). The API on AMQQueue should potentially just be the ability to acquire a set of messages matching a filter. In that sense I think the change to make the subject queue manage the transaction is suspect. This is possibly worth a separate JIRA though.
        Hide
        Robbie Gemmell added a comment -

        I can agree with that...but yes, seperate JIRA

        Show
        Robbie Gemmell added a comment - I can agree with that...but yes, seperate JIRA
        Hide
        Robbie Gemmell added a comment -

        I'm just closing this out. If the queue depth check ever starts being a problem im happy that its an easy NO-JIRA fix.

        Show
        Robbie Gemmell added a comment - I'm just closing this out. If the queue depth check ever starts being a problem im happy that its an easy NO-JIRA fix.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development