[jira] [Commented] (QPIDJMS-121) Adds ability to acknowledge amqp messages with different ack types

["Robbie Gemmell (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/QPIDJMS-121?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14950297#comment-14950297 ] 

Robbie Gemmell commented on QPIDJMS-121:
----------------------------------------

Hi Andrew,


Regarding your comment on the mailing list, it isn't typically necessary to await approval to send a Pull Request. In terms of your ICLA, I can see it has now been processed and is on record. In terms of particular changes themselves there is also usually no need to wait, raising a PR isn't much different to raising a JIRA \[with a patch\], and can always be closed out in the end if need be. For new features its often good to discuss their approach in advance, in which case there shouldn't be much surprise if a PR pops up. A PR can also be good for doing some review, or alternatively there is a ReviewBoard instance available for the same if not using GitHub.

I have now taken a quick look at the changes. As a bit of background, one of the things we are working on in the AMQP Bindings & Mappings TC at OASIS is a mapping for JMS, with the client serving as an implementation of that mapping as both progress. Something we are keen to do wherever possible is stick to the JMS API and avoid implementing AMQP specific behaviours using extension methods, due to this being rather implementation-specific and non-portable. There aren't many other options with behaviour like this, but I would want to discuss it with folks to determine that there really isn't a suitable alternative before proceeding.

If we were to proceed with such a change, there are some updates that I think would be needed first:

- ACK_TYPE wasnt really intended to be surfaced to an application, and I'm not sure its entirely suitable for doing so. It has been used under the covers previously and can and already has changed in the past, plus some values may not be suitable for the way it would then be getting used (e.g delivered). I think using int flags might also be favourable for consistency with the rest of the client API, these could be declared constants alongside the method definition.
- Use of POISONED to provoke a Rejected disposition. The client already uses that value elsewhere to provoke a Modified(delivery-failed,undeliverable-here) disposition, and using it for multiple different behaviours seems unwise. If the general functionality is to be thought of as 'AMQP style acks' it should probably just cover each of the dispositions rather than just some of them, so folks can use whichever they need to. The constant names could be aligned with the dispositions.
- There aren't any tests for the new behaviour. Since no normal/JMS things would be using this extra functionality during regular build/usage, without any tests it would be prone to breakage without realising. The client module has a 'test peer' that can be used to run the client up without a broker and play/validate AMQP level interactions against it. There are 'disposition matchers' already and probably some similar tests (see the *IntegrationTest classes in the client module) already for the regular Accepted case you could base on.
- I'm not a huge fan of having Amqp in the message interface name, it would be the only case I can think of at the perimeter of the clients JMS layer. Taking it further, I'd quite possibly even skip the interface and just use the JmsMessage object directly, since the result will be non-portable anyway which would become a little more clear, and I also wouldn't expect multiple implementations.
- There appears to be an unrelated change in the diff to the clients default idleTimeout config.

It would also be good if you rebased changes to be on top of the \[near-\]current codebase, i.e the the latest changes in your branch. Multiple commits on a PR is of course fine, but the changes ideally should be the most recent commits and exclude significant merges (such as the ~0.4.0 -> ~0.6.0 gap fixup) to make things easy to dig in to, and indeed bring to the main repo branch later. Adding the JIRA key to any commits (and any PR titles) will also help with keeping track of things later.

Robbie

> Adds ability to acknowledge amqp messages with different ack types
> ------------------------------------------------------------------
>
>                 Key: QPIDJMS-121
>                 URL: https://issues.apache.org/jira/browse/QPIDJMS-121
>             Project: Qpid JMS
>          Issue Type: Improvement
>          Components: qpid-jms-client
>    Affects Versions: 0.6.0
>            Reporter: Andrew Buckley
>
> description from GitHub README. README and code changes located here: https://github.com/andrew-buckley/qpid-jms/
> h1. QpidJMS with AMQP-style acknowledgements
> This is a fork of the QpidJMS project that adds the ability to acknowledge
> messages in three ways: CONSUMED, POISONED, and RELEASED.
> The fork is based on QpidJMS version 0.4.0-SNAPSHOT but has been merged with the current unreleased 0.6.0. The fork is
> fully compatible with JMS 1.1 but provides an additional interface, JmsAmqpMessage
> (found in org.apache.qpid.jms), that features this new functionality.
> Any messages consumed using this provider can be safely cast to type JmsAmpqMessage.
> h2. Additions
> * org.apache.qpid.jms.message.JmsAmqpMessage
> * org.apache.qpid.jms.message.JmsAcknowledgeCallback
> h2. Changes
> * Added method acknowledge(ProviderConstants.ACK_TYPE ackType) to org.apache.qpid.jms.message.JmsMessage.
> * Changed acknowledgeCallback type in org.apache.qpid.jms.message.JmsMessage to type JmsAcknowledgeCallback.
> * Changed method onInboundMessage in org.apache.qpid.jms.JmsMessageConsumer to set a message callback
> that takes an ACK_TYPE as an argument.
> * Changed method acknowledge() in org.apache.qpid.jms.JmsSession to take an ACK_TYPE as an argument.
> * Changed method acknowledge(JmsSessionId sessionId) in org.apache.qpid.jms.JmsConnection to take an ACK_TYPE
> as an argument.
> * Changed method acknowledge(final JmsSessionId sessionId, final AsyncResult request) in
> org.apache.qpid.jms.provider.amqp.AmqpProvider to take an ACK_TYPE as an argument.
> * Changed method acknowledge() in org.apache.qpid.jms.provider.amqp.AmqpSession to take an ACK_TYPE
> as an argument.
> * Changed method acknowledge() in org.apache.qpid.jms.provider.amqp.AmqpConsumer to take an ACK_TYPE
> as an argument. The method maps acknowledgement types to AMQP dispositions as follows:
> CONSUMED to ACCEPTED, POISONED to REJECTED, RELEASED to RELEASED.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org