spring-attic / spring-cloud-stream-binder-jms

Other
33 stars 56 forks source link

Added 'deadLetterQueueName' as configuration property. #19

Closed donovanmuller closed 7 years ago

donovanmuller commented 7 years ago

Instead of using a hardcoded value (ActiveMQ.DLQ) for the DLQ queue name, use a configuration property as a basic improvement.

garyrussell commented 7 years ago

Hi Donovan,

I was just wondering what was the rationale for making the producer/consumer destinations top level classes?

I agree that they should have been declared static since they are not dependent on the outer class, but I don't think they need to be in their own files.

If you can convince us that is necessary, we'll have to do the same in other binders for consistency.

cc/ @sobychacko

sobychacko commented 7 years ago

Looks like JMS case is a bit different. Various provisioning implementation might want to leverage on both JmsConsumerDestination and JmsProducerDestination. When we made the initial changes we only had Activemq provisioner to work with. Now that we have another provisioiner for ibm-mq, they could reuse them there as well. Thats the rationale I think for why this change is made. I don't think we have the same situation in other binders.

donovanmuller commented 7 years ago

@garyrussell The reason mentioned by @sobychacko is accurate. See here and here from https://github.com/spring-cloud/spring-cloud-stream-binder-ibm-mq/pull/2 for reference.

garyrussell commented 7 years ago

Doh yes, of course; merging with @mbogoevici 's suggested default for the DLQ.

garyrussell commented 7 years ago

Changes for review before I merge: https://github.com/garyrussell/spring-cloud-stream-binder-jms/commit/0a81989d80aaade234f101014b57f56603a940d3

@donovanmuller Please also note (in future) to limit commit headlines to 50 chars.

We generally use the commit guidelines from Pro Git.

(I will take care of it when squashing my commit during the merge after my changes are reviewed).

Thanks for the contribution!

garyrussell commented 7 years ago

Merged as d812f4ef63f30c6c5e1c0245bdec59c877e8a8e3