[GitHub] [activemq-artemis] assens commented on a change in pull request #2837: ARTEMIS-2476: New MQTT subscriptions receive older (not last published) retained message.
assens commented on a change in pull request #2837: ARTEMIS-2476: New MQTT
subscriptions receive older (not last published) retained message.
URL: https://github.com/apache/activemq-artemis/pull/2837#discussion_r324993925
##
File path:
artemis-protocols/artemis-mqtt-protocol/src/test/java/org/apache/activemq/artemis/core/protocol/mqtt/MqttClientService.java
##
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.protocol.mqtt;
+
+import static java.util.Objects.nonNull;
+import static
org.eclipse.paho.client.mqttv3.MqttConnectOptions.MQTT_VERSION_3_1_1;
+
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.function.Consumer;
+
+import javax.annotation.PostConstruct;
+import javax.annotation.PreDestroy;
+
+import org.eclipse.paho.client.mqttv3.IMqttDeliveryToken;
+import org.eclipse.paho.client.mqttv3.MqttCallback;
+import org.eclipse.paho.client.mqttv3.MqttClient;
+import org.eclipse.paho.client.mqttv3.MqttConnectOptions;
+import org.eclipse.paho.client.mqttv3.MqttException;
+import org.eclipse.paho.client.mqttv3.MqttMessage;
+import org.eclipse.paho.client.mqttv3.persist.MemoryPersistence;
+import org.jboss.logging.Logger;
+
+import io.netty.util.concurrent.DefaultThreadFactory;
+
+public class MqttClientService implements MqttCallback {
Review comment:
Hi. The MQTTRetainMessageManagerTest uses [Eclipse
Paho](https://www.eclipse.org/paho/) MQTT client to connect to Artemis.
MqttCallback is an interface defined by the Paho client, that provides a call
back when a message is received.
The MqttClientService used by the MQTTRetainMessageManagerTest uses the Paho
MQTT client to publish messages and at the same time implements the
MqttCallback to be able to subscribe and receive messages.
The MQTTRetainMessageManagerTest uses 4 instances of MqttClientService.
- mqttConsumerCount is used only to count the messages received by it as
consumer
- mqttConsumerBeforePublish is a consumer which subscribes to the fact
topic, before any publishing starts.
- arrivedCountAferPublish and arrivedCountAferPublish2 are consumers that
subscribe after messages are published.
See how the actAndAssert method uses the 4 clients.
The goal of the test is to assert that retained message received by all
client subscriptions (before and after the publish) is equal to the last
retained message published.
In addition, I've created a [mqtt-test](https://github.com/assens/mqtt-test)
project that demonstrates the problem we have with Artemis, and also shows that
other brokers such as Ejabberd, Emqx, HiveMQ, Moquette, Mosquitto, RabbitMQ and
VerneMQ don't have such problem.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] gaohoward commented on issue #2840: ARTEMIS-2493 OpenWire session close doesn't cleanup consumer refs
gaohoward commented on issue #2840: ARTEMIS-2493 OpenWire session close doesn't cleanup consumer refs URL: https://github.com/apache/activemq-artemis/pull/2840#issuecomment-532044831 thanks @clebertsuconic 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #2840: ARTEMIS-2493 OpenWire session close doesn't cleanup consumer refs
clebertsuconic commented on issue #2840: ARTEMIS-2493 OpenWire session close doesn't cleanup consumer refs URL: https://github.com/apache/activemq-artemis/pull/2840#issuecomment-532043791 I'm running whole testsuite before we can merge this. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request #2841: Simplifying Scale down remove queue logic
clebertsuconic opened a new pull request #2841: Simplifying Scale down remove queue logic URL: https://github.com/apache/activemq-artemis/pull/2841 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] gaohoward opened a new pull request #2840: ARTEMIS-2493 OpenWire session close doesn't cleanup consumer refs
gaohoward opened a new pull request #2840: ARTEMIS-2493 OpenWire session close doesn't cleanup consumer refs URL: https://github.com/apache/activemq-artemis/pull/2840 When an openwire client closes the session, the broker doesn't clean up its server consumer references even though the core consumers are closed. This results a leak when sessions within a connection are created and closed when the connection keeps open. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #2837: ARTEMIS-2476: New MQTT subscriptions receive older (not last published) retained message.
clebertsuconic commented on a change in pull request #2837: ARTEMIS-2476: New
MQTT subscriptions receive older (not last published) retained message.
URL: https://github.com/apache/activemq-artemis/pull/2837#discussion_r324710564
##
File path:
artemis-protocols/artemis-mqtt-protocol/src/test/java/org/apache/activemq/artemis/core/protocol/mqtt/MqttClientService.java
##
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.protocol.mqtt;
+
+import static java.util.Objects.nonNull;
+import static
org.eclipse.paho.client.mqttv3.MqttConnectOptions.MQTT_VERSION_3_1_1;
+
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.function.Consumer;
+
+import javax.annotation.PostConstruct;
+import javax.annotation.PreDestroy;
+
+import org.eclipse.paho.client.mqttv3.IMqttDeliveryToken;
+import org.eclipse.paho.client.mqttv3.MqttCallback;
+import org.eclipse.paho.client.mqttv3.MqttClient;
+import org.eclipse.paho.client.mqttv3.MqttConnectOptions;
+import org.eclipse.paho.client.mqttv3.MqttException;
+import org.eclipse.paho.client.mqttv3.MqttMessage;
+import org.eclipse.paho.client.mqttv3.persist.MemoryPersistence;
+import org.jboss.logging.Logger;
+
+import io.netty.util.concurrent.DefaultThreadFactory;
+
+public class MqttClientService implements MqttCallback {
Review comment:
Can you explain me what is this? just to make it easier for me to understand?
Is this a server with only the MQTT portion?
I can certainly hack your code and try to udnerstand.. just asking some
minimal english explanation to make my review easier.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 commented on a change in pull request #2832: ARTEMIS-2482 Large
messages could leak native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#discussion_r324693072
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/LargeServerMessageImpl.java
##
@@ -511,6 +512,44 @@ protected void closeFile() throws Exception {
}
}
+ private static int read(final SequentialFile file, final ByteBuffer
bufferRead) throws Exception {
Review comment:
@wy96f
It was the second point on my list :)
Yes, it can be done, but deserve lot of attention and probably some changes
on the API on SessionCallback...
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] assens commented on issue #2837: ARTEMIS-2476: New MQTT subscriptions receive older (not last published) retained message.
assens commented on issue #2837: ARTEMIS-2476: New MQTT subscriptions receive older (not last published) retained message. URL: https://github.com/apache/activemq-artemis/pull/2837#issuecomment-531769456 I've provided the MQTTRetainMessageManagerTest which asserts that retained messages received on a newly created subscriptions are matching the last published message on the given topic. @clebertsuconic 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] asfgit merged pull request #2838: NO-JIRA Add 2.10.0 entry with upgrade instructions
asfgit merged pull request #2838: NO-JIRA Add 2.10.0 entry with upgrade instructions URL: https://github.com/apache/activemq-artemis/pull/2838 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] asfgit closed pull request #2838: NO-JIRA Add 2.10.0 entry with upgrade instructions
asfgit closed pull request #2838: NO-JIRA Add 2.10.0 entry with upgrade instructions URL: https://github.com/apache/activemq-artemis/pull/2838 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] wy96f commented on a change in pull request #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
wy96f commented on a change in pull request #2832: ARTEMIS-2482 Large messages
could leak native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#discussion_r324656398
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/LargeServerMessageImpl.java
##
@@ -511,6 +512,44 @@ protected void closeFile() throws Exception {
}
}
+ private static int read(final SequentialFile file, final ByteBuffer
bufferRead) throws Exception {
Review comment:
@franz1981 Given this is a hot path and in most
cases(LargeMessageDeliverer::deliver,
ClientProducerImpl::largeMessageSendServer, CoreMessage::getLargeMessageBuffer,
etc) "bufferRead" is a heap buffer, can we construct "bufferRead" by using
PooledByteBufAllocator before calling to save from coping buffer?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] wy96f commented on a change in pull request #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
wy96f commented on a change in pull request #2832: ARTEMIS-2482 Large messages
could leak native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#discussion_r324656398
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/LargeServerMessageImpl.java
##
@@ -511,6 +512,44 @@ protected void closeFile() throws Exception {
}
}
+ private static int read(final SequentialFile file, final ByteBuffer
bufferRead) throws Exception {
Review comment:
Given this is a hot path and in most cases(LargeMessageDeliverer::deliver,
ClientProducerImpl::largeMessageSendServer, CoreMessage::getLargeMessageBuffer,
etc) "bufferRead" is a heap buffer, can we construct "bufferRead" by using
PooledByteBufAllocator before calling to save from coping buffer?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] MrEasy opened a new pull request #2839: [ARTEMIS-2490] Prevent NumberFormatExc when reading large message
MrEasy opened a new pull request #2839: [ARTEMIS-2490] Prevent NumberFormatExc when reading large message URL: https://github.com/apache/activemq-artemis/pull/2839 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis-native] MrEasy commented on issue #4: ARTEMIS-2426 Added Bundle-Native code header
MrEasy commented on issue #4: ARTEMIS-2426 Added Bundle-Native code header URL: https://github.com/apache/activemq-artemis-native/pull/4#issuecomment-531727160 @clebertsuconic Could you consider this for inclusion in next Artemis release? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] assens commented on issue #2837: ARTEMIS-2476: New MQTT subscriptions receive older (not last published) retained message.
assens commented on issue #2837: ARTEMIS-2476: New MQTT subscriptions receive older (not last published) retained message. URL: https://github.com/apache/activemq-artemis/pull/2837#issuecomment-531716019 > Whilst im not an mqtt person so cannot review code from a correctness pov, will need someone else to. As a standard requirement can a test case please be added that exposes the issues and proves the fix. I have created a project which demonstrates the problem. It's described in the related issue https://issues.apache.org/jira/browse/ARTEMIS-2476 I'll try to create a relevant unit test inside the artemis-mqtt-protocol maven module. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] bs-logisoft opened a new pull request #2838: NO-JIRA Add 2.10.0 entry with upgrade instructions
bs-logisoft opened a new pull request #2838: NO-JIRA Add 2.10.0 entry with upgrade instructions URL: https://github.com/apache/activemq-artemis/pull/2838 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2837: ARTEMIS-2476: New MQTT subscriptions receive older (not last published) retained message.
michaelandrepearce commented on issue #2837: ARTEMIS-2476: New MQTT subscriptions receive older (not last published) retained message. URL: https://github.com/apache/activemq-artemis/pull/2837#issuecomment-531705224 Whilst im not an mqtt person so cannot review code from a correctness pov, will need someone else to. As a standard requirement can a test case please be added that exposes the issues and proves the fix. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] lulf commented on issue #2834: ARTEMIS-2488: Handle the case where source address is null
lulf commented on issue #2834: ARTEMIS-2488: Handle the case where source address is null URL: https://github.com/apache/activemq-artemis/pull/2834#issuecomment-531700383 @clebertsuconic @michaelandrepearce Updated with your suggestions. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-531690229 @wy96f fair enough :) Still waiting some results from the integration tests on the CI and will be ready to be merged, although I prefer to improve code quality of this PR, there are some changes due to JDBC that makes is uglier than I wished 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] wy96f commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
wy96f commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-531688339 As you said, size of tiny/small/medium caches is quite small. Even if not freed in time, the impact is very little compared to IOUtil that pool native ByteBuffers(default TEMP_BUF_POOL_SIZE=8) for each thread :) 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] wy96f commented on issue #2836: [ARTEMIS-2487] Updated to org.jctools:jctools-core:2.1.2
wy96f commented on issue #2836: [ARTEMIS-2487] Updated to org.jctools:jctools-core:2.1.2 URL: https://github.com/apache/activemq-artemis/pull/2836#issuecomment-531677394 Good job! 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] assens opened a new pull request #2837: fix for #ARTEMIS-2476
assens opened a new pull request #2837: fix for #ARTEMIS-2476 URL: https://github.com/apache/activemq-artemis/pull/2837 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
