gemmellr commented on code in PR #4418:
URL: https://github.com/apache/activemq-artemis/pull/4418#discussion_r1151687346
##########
tests/soak-tests/pom.xml:
##########
@@ -260,7 +260,7 @@
<configuration>${basedir}/target/classes/servers/interruptlm</configuration>
<args>
<arg>--java-options</arg>
- <arg>-Djava.rmi.server.hostname=localhost</arg>
+ <arg>-Djava.rmi.server.hostname=localhost
-DARTEMIS_DEBUG_REF=true</arg>
Review Comment:
Again, seems odd to have the soak tests default to running things in a
typically non-representative manner. Perhaps more so in this case than the
leak-tests, given none of these tests are actually even looking at/using the
effect of the config.
Perhaps just add some mechanism to do this easily on request, e.g a property
that defaults empty so you can ask the tests to do this without modifying them?
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -16,12 +16,56 @@
*/
package org.apache.activemq.artemis.api.core;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.lang.invoke.MethodHandles;
+import java.lang.ref.Cleaner;
+import java.util.ArrayList;
import java.util.HashMap;
import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
-// import org.apache.activemq.artemis.utils.collections.ConcurrentHashSet; --
#ifdef DEBUG
+import org.apache.activemq.artemis.core.client.ActiveMQClientLogger;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class RefCountMessage {
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private static final Cleaner CLEANER;
+ public static final boolean DEBUG_REF_COUNT;
+
+ private static class RefCountDebugStatus implements Runnable {
+
+ RefCountDebugStatus(String clazz) {
+ this.clazz = clazz;
Review Comment:
Using 'clazz' as the name seems more typical for Class objects. Perhaps
'className' given its a string?
##########
tests/leak-tests/pom.xml:
##########
@@ -246,7 +246,8 @@
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<skipTests>${skipLeakTests}</skipTests>
-
<argLine>-agentpath:${project.basedir}/target/lib/${check-leak-deploy-name}
-Djgroups.bind_addr=::1 ${activemq-surefire-argline}
-Dorg.apache.activemq.SERIALIZABLE_PACKAGES="java.lang,javax.security,java.util,org.apache.activemq,org.fusesource.hawtbuf"</argLine>
+ <!-- -DARTEMIS_DEBUG_REF is needed for RefCountMessageLeakTest
-->
+
<argLine>-agentpath:${project.basedir}/target/lib/${check-leak-deploy-name}
-DARTEMIS_DEBUG_REF=true -Djgroups.bind_addr=::1 ${activemq-surefire-argline}
-Dorg.apache.activemq.SERIALIZABLE_PACKAGES="java.lang,javax.security,java.util,org.apache.activemq,org.fusesource.hawtbuf"</argLine>
Review Comment:
Seems wrong to enable it for all of them, meaning all the tests then aren't
checking the system in the manner it will be used normally, just so one test
checks it in that state. Maybe add a separate surefire execution with the
necessary config for this specific test?
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -49,6 +93,28 @@ public int getDurableCount() {
return DURABLE_REF_COUNT_UPDATER.get(this);
}
+ /** Sub classes may mark fired=true when they were explicitly called.
+ * E.g large message removed the file upon cancellation */
+ protected void markFired() {
+ fired = true;
Review Comment:
It feels like this variable is now being used in two different ways along
with a similar-but-separate variable of the same name elsewhere (in the
debugStatus), which all seems a bit non-obvious and the overall 'expected
normal behaviour' here is really quite hard to see from the code to me as a
result.
Previously it looks like this 'fired' variable only governed whether
onDown() would call the releaseComplete() method, i.e it tracked if
releaseComplete() was already fired (albeit it would still allow it to happen
concurrently). The variable still does that in terms of its use in onDown(),
but that method doesnt touch the separately added 'debugStatus fired' field
which this markFired() method does _also_ touch here. However its that latter
separate field that is apparently all the cleaner runnable looks at before
logging non-cleanup of a message? Is that right? Either way feels unnecessarily
hard to follow, at the very least some renaming of the method and variables
might help, the 'fired' in all the method and variables names seem like they
could be made more specific for clarity.
--
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]
