Add preserveDeliveryMode to deadLetterStrategy

[pa-deasy]

What problems does this PR solve? By default non persistent messages are set to persistent when being sent to a dead letter queue. This improvement adds preserveDeliveryMode to deadLetterStrategy which allows the original non-persistent delivery mode to be preserved.

Why is it beneficial to merge into ActiveMQ? Users are able to take advantage of using non persistent messages on a dead letter queue.

How do you make sure this PR is well tested? I ran the following tests locally on a build of this branch.

Test A

• Created a queue deadLetterStrategy to process non persistent messages.

<policyEntry queue=">">
  <deadLetterStrategy>
     <sharedDeadLetterStrategy processNonPersistent="true" />
   </deadLetterStrategy>
</policyEntry>

• Used a client to send a persistent message a queue, rolling back the session and causing the message to be redirected to a DLQ.
• Verified via the web console that the DLQ message had Persistence set to Persistent.

Test B

• Use the same deadLetterStrategy from Test A.
• Used a client to send a non persistent message a queue, rolling back the session and causing the message to be redirected to a DLQ.
• Verified via the web console that the DLQ message had Persistence set to Persistent and originalDeliveryMode set to NON_PERSISTENT.

Test C

• Updated the queue deadLetterStrategy to preserve the delivery mode.

<policyEntry queue=">">
  <deadLetterStrategy>
     <sharedDeadLetterStrategy processNonPersistent="true" preserveDeliveryMode="true" />
   </deadLetterStrategy>
</policyEntry>

• Used a client to send a non persistent message a queue, rolling back the session and causing the message to be redirected to a DLQ.
• Verified via the web console that the DLQ message had Persistence set to Non Persistent.

[kenliao94]

@pa-deasy I assume the code that actually calls "setPreserveDeliveryMode" which is read from destination configuration will come later?

[kenliao94]

I sort of see the points of moving the code to AbstractDeadLetterStrategy#prepareMessageForDeadLetterQueuefor readability in the chunky RegionBroker. My concern of that approach is that it effectively shifts the responsibility of preparing the message (setting correct attributes of the message object) to the this abstract class, which imo shouldn't need to be concern of it. This preparation is necessary before you send the message, but by moving to the strategy class, it feels like it's more a "util" then a necessary step. Furthermore, DeadLetterStrategy should have logic that only perform mutable action on the policy itself and shouldn't perform mutable actions on the Message object.

Hence I vote -1 on "move dead letter queue message preparation logic to the AbstractDeadLetterStrategy class" part of this PR. The getter and setter of perserveDeliveryMode on the Strategy class makes sense to me.

[mattrpav]

I'm not in favor of refactoring RegionBroker in a point release. Ultimately, the DeadLetterStrategy should re-use the MessageInterceptorStrategy interface to it so admins can configure or change anything about the message on the way to the DLQ. All these one-off config params and methods creates bloat and all this logic should be pushed off into a handler.

[pa-deasy]

@pa-deasy I assume the code that actually calls "setPreserveDeliveryMode" which is read from destination configuration will come later? Yes that should be straightforward. I just wanted feedback on the RegionBroker and DeadLetterStrategy` for the draft.

Thanks folks for your feedback on "move dead letter queue message preparation logic to the AbstractDeadLetterStrategy class". Those are great points. I will update my PR appropriately and add tests.

Thank you.

[cshannon]

@mattrpav - Do you have any issues with the changes here? It seems ok to me, it preserves the previous behavior by default and I could see a use case for this.

[jbonofre]

I will create a Jira associated with the change. The PR description is well detailed though.