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]

The comment needs to be updated.

[pa-deasy]

@return the PreserveDeliveryMode

This is a new method which does return PreserveDeliveryMode. What exactly would you like to update?

[kenliao94]

Ah, I guess just the name is a bit confusing to me. Because it starts with "is" and return a boolean, if it returns the PreserveDeliveryMode it should be "get" imo.

[pa-deasy]

Yeah, I noticed the "is" style is used for boolean methods in a number of places so I chose to be consistent.

[Nikita-Shupletsov]

yeah, is for boolean is the convention

[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.

[Nikita-Shupletsov]

preserveDeliveryMode

[pa-deasy]

Good spot, not sure how I made this mistake.

[Nikita-Shupletsov]

yeah, is for boolean is the convention

[cshannon]

I don't think we need/should create a new method here. This method is only used one time so I don't see the benefit of refactoring this unless you are planning to call prepareMessageForDeadLetterQueue() in more than one spot or make it protected/package scope to override it or be accessible in tests so I would just leave things inline.

[pa-deasy]

I thought refactoring out made the prepareMessageForDeadLetterQueue method made the sendToDeadLetterQueue method easier to read and understand.
If folks don't agree I can revert the change?

[cshannon]

Well, while the method is in fact descriptive about what it is doing, it actually makes it harder to understand what you changed because by copying it all to a new method it's not clear what exactly the difference is from the old code. You have to look at it carefully as the entire method shows up as a new change which is the biggest reason why I made the comment because i as staring at it for a while to even figure out what was different.

That being said it's not the biggest deal obviously, the compiler may end up inlining the method anyways.

[pa-deasy]

Makes sense, I've removed the prepareMessageForDeadLetterQueue method.

[kenliao94]

It should have the same format as other getter for consistency. So return at new new line

[pa-deasy]

Strange, my IDE(ItelliJ) made the other getters in AbstractDeadLetterStrategy appear to be single line.
I'll update this.

[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.