[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] 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:
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] 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
[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-531656312 @wy96f > So the cache will be freed and used by other threads when thread dies? Yes, it is, but my concern is that it needs a finalization run before getting there ie not that deterministic as I wish :) 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-531654209
@franz1981
> I have to add more detail and I was wrong: the tiny/small/medium thread
local caches are actually leaking memory regions (with default
io.netty.allocator.useCacheForAllThread) that need the holding thread to die in
order to make them available to other threads again
I assumed memory released into tiny/small/medium cache would be leaked if
holding thread terminates until I looked at the code in PoolThreadCache:
```
/// TODO: In the future when we move to Java9+ we should use
java.lang.ref.Cleaner.
@Override
protected void finalize() throws Throwable {
try {
super.finalize();
} finally {
free();
}
}
/**
* Should be called if the Thread that uses this cache is about to
exist to release resources out of the cache
*/
void free() {
// As free() may be called either by the finalizer or by
FastThreadLocal.onRemoval(...) we need to ensure
// we only call this one time.
if (freed.compareAndSet(false, true)) {
int numFreed = free(tinySubPageDirectCaches) +
free(smallSubPageDirectCaches) +
free(normalDirectCaches) +
free(tinySubPageHeapCaches) +
free(smallSubPageHeapCaches) +
free(normalHeapCaches);
if (numFreed > 0 && logger.isDebugEnabled()) {
logger.debug("Freed {} thread-local buffer(s) from thread:
{}", numFreed,
Thread.currentThread().getName());
}
if (directArena != null) {
directArena.numThreadCaches.getAndDecrement();
}
if (heapArena != null) {
heapArena.numThreadCaches.getAndDecrement();
}
}
}
```
So the cache will be freed and used by other threads when thread dies? If
so, it seems no any leak problems with this pr, no need with improvements?
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
