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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
