[
https://issues.apache.org/jira/browse/QPIDJMS-608?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17952249#comment-17952249
]
David Ansari commented on QPIDJMS-608:
--------------------------------------
Many thanks for your prompt reply.
> If the JMS sender is configured to send PERSISTENT then the header is
> included is it not?
Yes, correct, the header is included then. However, what I suggest is that the
header should also be included if the JMS sender is configured to send as
NON_PERSISTENT.
> as far as I can tell the library is following it.
That's correct. There is no bug in this library. I'm suggesting "only" an
improvement.
> The encoding and decoding of the header is measurable over time
I doubt that my proposed change will worsen performance. I'm happy to do some
performance comparison before/after against Qpid Broker-J or ActiveMQ Artemis
or some other broker. (Yet another option could be to configure this client
globally to opt in for my newly proposed behaviour.)
> If the RabbitMQ broker needs the header to function then it could create and
> insert one when decoding an incoming message instead of trying to enforce
> other libraries to do that work for it.
How? When RabbitMQ receives a message without AMQP 1.0 header section, it
assumes that durable=true (and it does so for the reasons mentioned above, i.e.
to be safe by default, that's our lesson learned from the last 15 years of
production experience). So, when RabbitMQ receives a message without header
section, it simply cannot differentiate between "AMQP 1.0 client app just
forgot to set the durable field to true" vs. "AMQP 1.0 client app explicitly
decided to opt in to allow the message to be lost (durable=false)".
> Send AMQP header section with durable field
> -------------------------------------------
>
> Key: QPIDJMS-608
> URL: https://issues.apache.org/jira/browse/QPIDJMS-608
> Project: Qpid JMS
> Issue Type: Improvement
> Components: qpid-jms-client
> Affects Versions: 2.7.0
> Reporter: David Ansari
> Priority: Major
> Fix For: 2.8.0
>
>
> I would like to suggest the following improvement.
> h1. What?
> The Qpid JMS client should always send the AMQP 1.0 header section with at
> least the durable field explicitly set to true or false.
> h1. Why?
> The current behaviour is that the header section is completely omitted from
> being sent (transmitted across the wire), if the JMS app opted in to send the
> message as NON_PERSISTENT. This complies with the AMQP 1.0 spec and is
> functionally correct because all the AMQP 1.0 header section field defaults
> will apply at the receiver.
> However, the AMQP 1.0 spec also states the following:
> "The header section carries standard delivery details about the transfer of a
> message through the AMQP network. If the header section is omitted the
> receiver MUST assume the appropriate default values (or the meaning implied
> by no value being set) for the fields within the header {*}unless other
> target or node specific defaults have otherwise been set{*}."
> This means that AMQP 1.0 brokers could set other defaults.
> RabbitMQ interprets {{durable}} as {{true}} by default to be safe by default.
> That's because beginners could easily forget setting the {{durable}} field to
> {{true}} resulting in unexpected message loss.
> In my personal opinion, the fact that the AMQP 1.0 spec defines the default
> for durable as false was a mistake:
> <field name="durable" type="boolean" default="false"/>
> In my opinion, JMS got it right by using a default of PERSISTENT for the
> message producer.
> Anyway, I suggest that this JMS client should always explicitly send the
> {{durable}} field to allow RabbitMQ to store the message non-durably. In
> addition, message selectors can filter on JMSDeliveryMode. Therefore,
> RabbitMQ needs to know whether durable is true or false.
> In other words, do not omit this field when marshalling.
> The performance penalty of sending the durable field is negligible.
> h1. How?
> I tested that the following patch works as expected:
> {code:java}
> diff --git
> a/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/message/AmqpHeader.java
>
> b/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/message/AmqpHeader.java
> index 622b5e4c..7deab521 100644
> ---
> a/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/message/AmqpHeader.java
> +++
> b/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/message/AmqpHeader.java
> @@ -75,26 +75,17 @@ public final class AmqpHeader {
> } public Header getHeader() {
> - Header result = null;
> -
> - if (!isDefault()) {
> - result = new Header();
> - // As we are now definitely sending a Header, always
> - // populate the durable field explicitly rather than
> - // potentially default it if false.
> - if(Boolean.TRUE.equals(durable)) {
> - result.setDurable(Boolean.TRUE);
> - }
> - else {
> - result.setDurable(Boolean.FALSE);
> - }
> -
> - result.setPriority(priority);
> - result.setFirstAcquirer(firstAcquirer);
> - result.setTtl(timeToLive);
> - result.setDeliveryCount(deliveryCount);
> + Header result = new Header();
> + if(Boolean.TRUE.equals(durable)) {
> + result.setDurable(Boolean.TRUE);
> }
> -
> + else {
> + result.setDurable(Boolean.FALSE);
> + }
> + result.setPriority(priority);
> + result.setFirstAcquirer(firstAcquirer);
> + result.setTtl(timeToLive);
> + result.setDeliveryCount(deliveryCount);
> return result;
> } {code}
>
> h1. Additional Context
> The same change was done in the AMQP 1.0 Go client:
> [https://github.com/Azure/go-amqp/issues/330]
>
> Please let us know what you think. We are happy to contribute a PR with tests
> directly to [https://github.com/apache/qpid-jms]
> Thank you for your collaboration!
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
