[
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: [email protected]
For additional commands, e-mail: [email protected]
