kenliao94 commented on PR #1354: URL: https://github.com/apache/activemq/pull/1354#issuecomment-2495775763
I sort of see the points of moving the code to `AbstractDeadLetterStrategy#prepareMessageForDeadLetterQueue`for 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. IMO `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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org For additional commands, e-mail: gitbox-h...@activemq.apache.org For further information, visit: https://activemq.apache.org/contact
