Qpid
  1. Qpid
  2. QPID-4164

management move/copy and max delivery attempt functionality fail to properly move/copy the message if it was loaded from the message store at startup

    Details

      Description

      The JMX moveMessage/copyMessage functions fail to properly move/copy the message if the message has been recovered from the store at Broker startup. The Broker permanently loses the message's payload.

      If an operator subsequently tries to view the problematic message with the Management interfaces, a JMX thread will be put into an infinite loop (see QPID-4170) and this will require a Broker restart.

      This problem does not affect messages sent during the lifetime of the Broker's execution (they can be moved or copied as normal). The existence of messages with lost payload within the Broker does not affect the flight of other messages through the Broker.

        Activity

        Hide
        Keith Wall added a comment -

        Patch applied, Phil/Robbie could you review this change please?

        Show
        Keith Wall added a comment - Patch applied, Phil/Robbie could you review this change please?
        Hide
        Robbie Gemmell added a comment -

        The changes look good to me except one small thing: in the Derby store it used to only keep the hard reference to the metadata if it still needed to be persisted whereas after the change it always does it, so recovered messages will always hard reference the metadata. I have restored the previous behaviour to the Derby store and added the same behaviour to the BDB store, allowing the SoftReference to release the data the same way it can for new messages arriving off the wire (if memory pressure demands it).

        Can you review my changes?

        (I would personally consider this to be a 0.18 blocker)

        Show
        Robbie Gemmell added a comment - The changes look good to me except one small thing: in the Derby store it used to only keep the hard reference to the metadata if it still needed to be persisted whereas after the change it always does it, so recovered messages will always hard reference the metadata. I have restored the previous behaviour to the Derby store and added the same behaviour to the BDB store, allowing the SoftReference to release the data the same way it can for new messages arriving off the wire (if memory pressure demands it). Can you review my changes? (I would personally consider this to be a 0.18 blocker)
        Hide
        Philip Harvey added a comment -

        This issue also affected messages that the broker moved to a DLQ, so a test should be added to check that this is fixed.

        Show
        Philip Harvey added a comment - This issue also affected messages that the broker moved to a DLQ, so a test should be added to check that this is fixed.
        Hide
        Philip Harvey added a comment -

        enhance MaxDeliveryCountTest to check that this problem no longer affects DLQs

        Show
        Philip Harvey added a comment - enhance MaxDeliveryCountTest to check that this problem no longer affects DLQs
        Hide
        Philip Harvey added a comment -

        attached patch to extend MaxDeliveryCountTest to check for this bug - please review and commit if you're happy.

        Show
        Philip Harvey added a comment - attached patch to extend MaxDeliveryCountTest to check for this bug - please review and commit if you're happy.
        Hide
        Keith Wall added a comment -

        Hi Robbie, I reviewed your change of 27/Jul/12 11:28. No comments. I agree these commits should be part of 0.18.

        Show
        Keith Wall added a comment - Hi Robbie, I reviewed your change of 27/Jul/12 11:28. No comments. I agree these commits should be part of 0.18.
        Hide
        Keith Wall added a comment -

        Hi Phil, no comments on your patch. It has been applied to trunk.

        Show
        Keith Wall added a comment - Hi Phil, no comments on your patch. It has been applied to trunk.
        Hide
        Justin Ross added a comment -

        Reviewed by Keith, Philip, and Robbie. Approved for 0.18.

        Show
        Justin Ross added a comment - Reviewed by Keith, Philip, and Robbie. Approved for 0.18.
        Hide
        Robbie Gemmell added a comment -

        Changes merged to the 0.18 release branch.

        Show
        Robbie Gemmell added a comment - Changes merged to the 0.18 release branch.
        Hide
        Robbie Gemmell added a comment -

        Attaching a patch for 0.16, should anyone need to resolve this issue for an existing system.

        It is slightly different than the trunk/0.18 changes in that it doesn't contain the metadata reference optimisation I mentioned above (to keep the change as isolated to the actual defect as possible) and lacks some of the tests added on trunk (but has enough to expose the issue and see that it is resolved).

        Show
        Robbie Gemmell added a comment - Attaching a patch for 0.16, should anyone need to resolve this issue for an existing system. It is slightly different than the trunk/0.18 changes in that it doesn't contain the metadata reference optimisation I mentioned above (to keep the change as isolated to the actual defect as possible) and lacks some of the tests added on trunk (but has enough to expose the issue and see that it is resolved).

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development