gemmellr commented on code in PR #4407:
URL: https://github.com/apache/activemq-artemis/pull/4407#discussion_r1140262169
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/jdbc/JdbcNodeManagerTest.java:
##########
@@ -56,29 +57,34 @@ public void configure() {
dbConf.setJdbcUser(getJdbcUser());
dbConf.setJdbcPassword(getJdbcPassword());
leaseLockExecutor = Executors.newSingleThreadScheduledExecutor();
+ runAfter(leaseLockExecutor::shutdownNow);
}
- @After
- public void shutdownExecutors() throws InterruptedException {
- try {
- final CountDownLatch latch = new CountDownLatch(1);
- leaseLockExecutor.execute(latch::countDown);
- Assert.assertTrue("the scheduler of the lease lock has some pending
task in ", latch.await(10, TimeUnit.SECONDS));
- } finally {
- leaseLockExecutor.shutdownNow();
- }
- }
- @After
@Override
- public void shutdownDerby() {
- try {
- if (useAuthentication) {
- DriverManager.getConnection("jdbc:derby:;shutdown=true",
getJdbcUser(), getJdbcPassword());
- } else {
- DriverManager.getConnection("jdbc:derby:;shutdown=true");
+ public void dropDerby() {
+ if (useAuthentication) {
+ try {
+ DriverManager.getConnection("jdbc:derby:" +
getEmbeddedDataBaseName() + ";drop=true", getJdbcUser(), getJdbcPassword());
+ } catch (Exception e) {
+ logger.info(e.getMessage());
+ }
+ try {
+
DriverManager.getConnection("jdbc:derby:;shutdown=true;deregister=false",
getJdbcUser(), getJdbcPassword());
+ } catch (Exception e) {
+ logger.info(e.getMessage());
+ }
+ } else {
+ try {
+ DriverManager.getConnection("jdbc:derby:" +
getEmbeddedDataBaseName() + ";drop=true");
+ } catch (Exception e) {
+ logger.info(e.getMessage());
+ }
+ try {
+
DriverManager.getConnection("jdbc:derby:;shutdown=true;deregister=false");
+ } catch (Exception e) {
+ logger.info(e.getMessage());
Review Comment:
If making commit message claims about doing 'a proper drop' could we please
start _actually_ doing it properly, by checking for the success codes in the
exceptions as per the documentation and example I noted yesterday. This is
still not doing it correctly. We can then omit the unnecessary info spam in
success cases, and actually throw in the non-success exception cases rather
than just logging both the same.
Also, could just call super for the basic non-authenticated case rather than
duplicate the behaviour.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/tests/util/ActiveMQTestBase.java:
##########
@@ -235,29 +222,40 @@ public MBeanServer getMBeanServer() {
// There is a verification about thread leakages. We only fail a single
thread when this happens
private static Set<Thread> alreadyFailedThread = new HashSet<>();
- private LinkedList<RunnableEx> runAfter;
+ private volatile List<RunnableEx> runAfter;
protected synchronized void runAfter(RunnableEx run) {
Assert.assertNotNull(run);
if (runAfter == null) {
- runAfter = new LinkedList();
+ runAfter = Collections.synchronizedList(new ArrayList<>());
}
runAfter.add(run);
}
@After
public void runAfter() {
- if (runAfter != null) {
- runAfter.forEach((r) -> {
+ List<RunnableEx> theRunafterList = runAfter;
Review Comment:
Perhaps localRunAfter[List] ?
##########
artemis-server/src/test/java/org/apache/activemq/artemis/tests/util/ActiveMQTestBase.java:
##########
@@ -235,29 +222,40 @@ public MBeanServer getMBeanServer() {
// There is a verification about thread leakages. We only fail a single
thread when this happens
private static Set<Thread> alreadyFailedThread = new HashSet<>();
- private LinkedList<RunnableEx> runAfter;
+ private volatile List<RunnableEx> runAfter;
protected synchronized void runAfter(RunnableEx run) {
Assert.assertNotNull(run);
if (runAfter == null) {
- runAfter = new LinkedList();
+ runAfter = Collections.synchronizedList(new ArrayList<>());
}
runAfter.add(run);
}
@After
public void runAfter() {
- if (runAfter != null) {
- runAfter.forEach((r) -> {
+ List<RunnableEx> theRunafterList = runAfter;
+ if (theRunafterList != null) {
+ // I do this replacement as eventually a runAfter lambda could be
adding another runAfter.
+ // this shouldn't happen very often but I saw it happening once.
+ // it's better to be protected against
Review Comment:
What did you see doing it? Is it really always invalid? If it is, then
rather than silently drop it like this does, it might be better to be aware of
it happening...e.g set a boolean noting the 'runAfter' things are starting to
execute and then it can at least throw or log if anything calls
runAfter(Runnable) to add a new task, and not add the task, rather than just
allowing it to create a new collection and then ignore its contents before
nulling it again as this change seems to.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/tests/util/ActiveMQTestBase.java:
##########
@@ -235,29 +222,40 @@ public MBeanServer getMBeanServer() {
// There is a verification about thread leakages. We only fail a single
thread when this happens
private static Set<Thread> alreadyFailedThread = new HashSet<>();
- private LinkedList<RunnableEx> runAfter;
+ private volatile List<RunnableEx> runAfter;
protected synchronized void runAfter(RunnableEx run) {
Assert.assertNotNull(run);
if (runAfter == null) {
- runAfter = new LinkedList();
+ runAfter = Collections.synchronizedList(new ArrayList<>());
}
runAfter.add(run);
}
@After
public void runAfter() {
- if (runAfter != null) {
- runAfter.forEach((r) -> {
+ List<RunnableEx> theRunafterList = runAfter;
+ if (theRunafterList != null) {
+ // I do this replacement as eventually a runAfter lambda could be
adding another runAfter.
+ // this shouldn't happen very often but I saw it happening once.
+ // it's better to be protected against
+ runAfter = null;
Review Comment:
If doing the above suggestion, this could just be moved to straight after
the initial local-assignment and done once regardless.
The runAfter() method should perhaps be synchronized, as the
runAfter(Runnable) method is and yet still isnt safe despite it due to this
nulling and the variable being made volatile.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/HAPolicyConfigurationTest.java:
##########
@@ -53,12 +53,20 @@
import org.apache.activemq.artemis.tests.util.ActiveMQTestBase;
import org.hamcrest.MatcherAssert;
import org.hamcrest.core.IsInstanceOf;
+import org.junit.After;
import org.junit.Test;
import static org.hamcrest.CoreMatchers.instanceOf;
public class HAPolicyConfigurationTest extends ActiveMQTestBase {
+ @After
+ public void tearDown() throws Exception {
Review Comment:
Missing override causing failure under errorprone.
--
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]
