Implement redelivery policy. #83
Conversation
|
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, |
Havret
left a comment
Havret
left a comment
There was a problem hiding this comment.
Besides what Robbie said, here are my two cents.
test/Apache-NMS-AMQP-Test/Integration/MessageRedeliveryPolicyIntegrationTest.cs
Outdated
Show resolved
Hide resolved
Regarding making the Outcome configurable are you happy for an extension to the NMS RedeliveryPolicy class to include a new Outcome property? |
|
Any idea when this pull request will be merged ? |
|
@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. |
Care to clarify-- specifically, what action or steps need to be taken for a +1? |
|
The first comment: #83 (comment) |
|
Just for your information, I was just trying to use this lib in my project at work and I needed the retry feature. |
|
Can confirm this works a lot better for individual ack. |
|
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 #83 (comment) With no changes, or responses to address those, the state of the pr hasn't changed |
|
Two years later... Can someone else champion this PR? |
|
Hi all is it worth me trying to resurrect this PR? The main issue was that I was asked to make it configurable. How do you want me to do that? "Regarding making the Outcome configurable are you happy for an extension to the NMS RedeliveryPolicy class to include a new Outcome property?" |
|
I'll try to take a look but can't promise an ETA. @stevemuk, please hold off on making any changes until I understand what's going on here. As others have pointed out, it's been quite a few years, and I need some time to get my head around it. |
There was a problem hiding this comment.
After some thought, here’s my proposal for how we can move this forward.
As @gemmellr suggested, we should not change the default behavior for rejection. I won’t go into detail or challenge his position — he’s one of the authors of both the AMQP spec and the JMS-AMQP spec, so he clearly knows what he’s talking about. Moreover, the current behavior aligns with what’s already implemented in qpid-jms.
That said, I also don’t want to force this behavior on users who may expect something slightly different. To support that, we should extend the IRedeliveryPolicy interface in the NMS.API project and introduce a new method that allows the provider to determine the appropriate acknowledgment type for expired messages. Since there’s already a default implementation in NMS.API, we’ll need to make the new method abstract and let each provider (e.g., AMQP, OpenWire) implement its own logic accordingly.
I’ve created a Jira issue to track this change: https://issues.apache.org/jira/browse/AMQNET-846.
The change that introduces message dispatch delay based on redelivery attempts should be handled separately and is outside the scope of the previous change. Personally, I’m not a big fan of this approach — I don’t believe a messaging library should be responsible for driving backoff or retry policies in the application. I also haven’t seen this implemented in Qpid JMS. However, based on feedback, it’s clear there’s strong demand for this feature among users of the AMQP provider.
⸻
Action points:
-
Create Jira for IRedeliveryPolicy extension in NMS.API - https://issues.apache.org/jira/browse/AMQNET-846
-
Create Jira for delayed message dispatch - https://issues.apache.org/jira/browse/AMQNET-847
-
@stevemuk – please clean up your PR and leave only the changes related to dispatch delay (use the proper commit message format with the Jira ID)
-
Implement changes in NMS.API - apache/activemq-nms-api#32
-
Release new version of NMS.API
-
Create a PR in NMS.AMQP to implement the new IRedeliveryPolicy behavior #95
-
Create a PR in NMS.OpenWire to implement the new IRedeliveryPolicy behavior
-
Release NMS.AMQP
-
Release NMS.OpenWire
Reject poison messages.
9 participants
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.