[
https://issues.apache.org/jira/browse/QPIDJMS-608?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
David Ansari updated QPIDJMS-608:
---------------------------------
Description:
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-durable. 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!
was:
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 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-durable. 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!
> 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-durable. 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
