[
https://issues.apache.org/jira/browse/ARTEMIS-4365?focusedWorklogId=871530&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-871530
]
ASF GitHub Bot logged work on ARTEMIS-4365:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 18/Jul/23 11:25
Start Date: 18/Jul/23 11:25
Worklog Time Spent: 10m
Work Description: gemmellr commented on code in PR #4556:
URL: https://github.com/apache/activemq-artemis/pull/4556#discussion_r1266568647
##########
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTPublishManager.java:
##########
@@ -404,12 +406,17 @@ private boolean publishToClient(int messageId,
ICoreMessage message, int deliver
// [MQTT-3.3.1-2] The DUP flag MUST be set to 0 for all QoS 0 messages.
boolean redelivery = qos == 0 ? false : (deliveryCount > 1);
- boolean isRetain = message.getBooleanProperty(MQTT_MESSAGE_RETAIN_KEY);
+ boolean isRetain = false;
Review Comment:
If it is primarily going to gate on
MQTT_MESSAGE_RETAIN_INITIAL_DISTRIBUTION_KEY presence for both protocols as
this does now, then could you not check for it once here and update isRetain
based on it? Seems like it would be simpler than doing it 3 times below. Would
allow dropping a couple else/else-if segments, leaving jsut the MQTT5-specific
check on the MQTT_MESSAGE_RETAIN_KEY if it wasnt already true.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/mqtt5/spec/controlpackets/PublishTests.java:
##########
@@ -512,7 +540,8 @@ public void testRetainHandlingTwo() throws Exception {
MqttClient consumer = createPahoClient(CONSUMER_ID);
consumer.setCallback(new DefaultMqttCallback() {
@Override
- public void messageArrived(String topic, MqttMessage message) throws
Exception {
+ public void messageArrived(String topic, MqttMessage message) {
+ assertTrue(message.isRetained());
Review Comment:
Seems superfluous when the test is already burning 2 seconds (ouch) to
verify this method isn't called.
Though if it wasnt, also seems odd the assertion is the opposite of the
previous test when it expected not to get the message.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/mqtt5/spec/controlpackets/PublishTests.java:
##########
@@ -481,7 +503,8 @@ public void messageArrived(String topic, MqttMessage
message) throws Exception {
final CountDownLatch latch2 = new CountDownLatch(1);
consumer.setCallback(new DefaultMqttCallback() {
@Override
- public void messageArrived(String topic, MqttMessage message) throws
Exception {
+ public void messageArrived(String topic, MqttMessage message) {
+ assertFalse(message.isRetained());
Review Comment:
Seems superfluous when the test is already burning 2 seconds (ouch) to
verify this method isn't called.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/mqtt/MQTTTest.java:
##########
@@ -2219,4 +2223,71 @@ public void testAutoDeleteRetainedQueue() throws
Exception {
Wait.assertTrue(() ->
server.locateQueue(RETAINED_QUEUE).getMessageCount() == 0, 3000, 50);
Wait.assertTrue(() -> server.locateQueue(RETAINED_QUEUE) == null, 3000,
50);
}
+
+ /*
+ * [MQTT-3.3.1-9] When sending a PUBLISH Packet to a Client the
Server...MUST set the RETAIN flag to 0 when a PUBLISH
+ * Packet is sent to a Client because it matches an *established*
subscription regardless of how the flag was set in
+ * the message it received.
+ */
+ @Test(timeout = 60 * 1000)
+ public void testRetainFlagOnEstablishedSubscription() throws Exception {
+ CountDownLatch latch = new CountDownLatch(1);
+ final String topic = RandomUtil.randomString();
Review Comment:
Can be nice to name things based on the test to help easily see later it was
for etc, useful for debugging.
E.g use, or add a prefix via, getTopicName() / getQueueName() / getName()
helpers.
Same for next test.
Issue Time Tracking
-------------------
Worklog Id: (was: 871530)
Time Spent: 20m (was: 10m)
> MQTT retain flag not set correctly
> ----------------------------------
>
> Key: ARTEMIS-4365
> URL: https://issues.apache.org/jira/browse/ARTEMIS-4365
> Project: ActiveMQ Artemis
> Issue Type: Bug
> Reporter: Justin Bertram
> Assignee: Justin Bertram
> Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> The {{retain}} flag set on MQTT messages dispatched to clients is incorrect
> in certain circumstances. This is demonstrated by using the {{test}} command
> from the [MQTT CLI tool|https://hivemq.github.io/mqtt-cli/docs/test/], e.g.:
> {noformat}
> $ mqtt test --all
> MQTT 3: OK
> - Maximum topic length: 65535 bytes
> - QoS 0: Received 100000/100000 publishes in 4847.01ms
> - QoS 1: Received 100000/100000 publishes in 27413.45ms
> - QoS 2: Received 100000/100000 publishes in 49551.40ms
> - Retain: OK
> - Wildcard subscriptions: OK
> - Shared subscriptions: OK
> - Payload size: >= 100000 bytes
> - Maximum client id length: 65535 bytes
> - Unsupported Ascii Chars: ALL SUPPORTED
> MQTT 5: OK
> - Connect restrictions:
> > Retain: OK
> > Wildcard subscriptions: OK
> > Shared subscriptions: OK
> > Subscription identifiers: OK
> > Maximum QoS: 2
> > Receive maximum: 65535
> > Maximum packet size: 268435455 bytes
> > Topic alias maximum: 65535
> > Session expiry interval: Client-based
> > Server keep alive: Client-based
> - Maximum topic length: 65535 bytes
> - QoS 0: Received 100000/100000 publishes in 706.21ms
> - QoS 1: Received 100000/100000 publishes in 805.38ms
> - QoS 2: Received 100000/100000 publishes in 972.98ms
> - Retain: TIME_OUT
> - Wildcard subscriptions: OK
> - Shared subscriptions: OK
> - Payload size: >= 100000 bytes
> - Maximum client id length: 65535 bytes
> - Unsupported Ascii Chars: ALL SUPPORTED{noformat}
> Notice the result of {{TIME_OUT}} when testing retain functionality for MQTT
> 5.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
