apache
Implement redelivery policy.
When setting a RedeliveryPolicy only the MaximumRedeliveries is honoured, RedeliveryDelay is neither calculated nor applied so retries are immediate rather than backing off. Secondly messages that fail to be processed are marked as MODIFIED_FAILED_UNDELIVERABLE rather than REJECTED, which leaves them on the queue until either they expire or the session is restarted.
This change will apply a RedeliveryDelay on retry and mark messages that exceed MaximumRedeliveries as REJECTED. I have tested this with AMQ Artemis and added unit tests.
I believe the NMS AMQP folks were basing their behaviour here on Qpid JMS like much of the code, in which case the MODIFIED_FAILED_UNDELIVERABLE (shorthand for undeliverable-here) is the expected and intended behaviour. Rejected has a particular meaning in the protocol that the underlying message is invalid, which means it is usually not the approripriate choice for simple 'nacking' (though annoyingly uses the more-obvious name).
Qpid JMS does have the ability to configured the outcome used, meaning it can be asked to do something else instead such as rejected if desired, I dont know if the NMS client does.
Youll need to raise a JIRA and update your commit to reference it (e.g see most prior commits). As you appear to be doing 2 seperate things you should probably have separate JIRAs and commits.
Hi @stevemuk, Thanks for your contribution. I will take a look at it over the weekend.
Besides what Robbie said, here are my two cents.
Regarding making the Outcome configurable are you happy for an extension to the NMS RedeliveryPolicy class to include a new Outcome property?
@stevemuk would you please rebase and squash down to one commit?
edit: tag the author
Squashing the commits isnt the holdup, and it is also someone elses PR.
The PR hasnt been merged as it simply isnt finished. The feeback I gave in July, around the original behaviour being intended, and the new behaviour implemented here for 'rejecting' actually being the wrong thing to do by default, wasnt yet actioned. Thats seemingly as an answer wasnt given to a question, one I dont know the answer to and so didnt answer.
Thats seemingly as an answer wasnt given to a question, one I dont know the answer to and so didnt answer.
Care to clarify-- specifically, what action or steps need to be taken for a +1?
The first comment: https://github.com/apache/activemq-nms-amqp/pull/83#issuecomment-1198049883 The last comment prior to todays question: https://github.com/apache/activemq-nms-amqp/pull/83#issuecomment-1216244962
Just for your information, I was just trying to use this lib in my project at work and I needed the retry feature. As it behaved strangely during my tests, I started to look around and found out this pull request. It seems to do what I wanted, but honestly, I don't know what I could do to help you on this as I don't know enough about the code in this repository... Anyway, thank you for your quick response :-)
Can confirm this works a lot better for individual ack. When using the main branch of this library, MaximumRedeliveries is respected, but messages are never sent to a dlq (and not marked as redelivered in the console, but i'm not sure if that is related). When using this pull request, the messages are put on a dlq when maximumredeliveries is reached
any help needed here ? Are we waiting for @Havret changes to be made ?
It's waiting on responses and corrections that Robbie previously highlighted. Robbie re called out those last yr on the 26 Oct 2022 https://github.com/apache/activemq-nms-amqp/pull/83#issuecomment-1292203011
With no changes, or responses to address those, the state of the pr hasn't changed
