gemmellr commented on code in PR #5160:
URL: https://github.com/apache/activemq-artemis/pull/5160#discussion_r1731005986
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/GlobalDiskFullTest.java:
##########
@@ -19,38 +19,99 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.lang.invoke.MethodHandles;
import java.net.URI;
+import java.util.Arrays;
+import java.util.Collection;
import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.activemq.artemis.core.config.Configuration;
import org.apache.activemq.artemis.core.server.ActiveMQServer;
import org.apache.activemq.artemis.core.server.files.FileStoreMonitor;
import org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl;
+import org.apache.activemq.artemis.core.settings.impl.AddressFullMessagePolicy;
+import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
+import org.apache.activemq.artemis.tests.extensions.parameterized.Parameter;
+import
org.apache.activemq.artemis.tests.extensions.parameterized.ParameterizedTestExtension;
+import org.apache.activemq.artemis.tests.extensions.parameterized.Parameters;
+import org.apache.activemq.artemis.utils.Wait;
import org.apache.activemq.transport.amqp.client.AmqpClient;
import org.apache.activemq.transport.amqp.client.AmqpConnection;
import org.apache.activemq.transport.amqp.client.AmqpMessage;
import org.apache.activemq.transport.amqp.client.AmqpSender;
import org.apache.activemq.transport.amqp.client.AmqpSession;
-import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@ExtendWith(ParameterizedTestExtension.class)
public class GlobalDiskFullTest extends AmqpClientTestSupport {
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ @Parameter(index = 0)
+ public AddressFullMessagePolicy addressFullPolicy;
+
+ @Parameters(name = "addressFullPolicy={0}")
+ public static Collection<Object[]> parameters() {
+ return Arrays.asList(new Object[][] {
+ {AddressFullMessagePolicy.FAIL}, {AddressFullMessagePolicy.DROP},
{AddressFullMessagePolicy.PAGE}
+ });
+ }
+
+ @Override
+ protected void configureAddressPolicy(ActiveMQServer server) {
+ AddressSettings addressSettings = new AddressSettings();
+ addressSettings.setAddressFullMessagePolicy(addressFullPolicy);
+ server.getConfiguration().addAddressSetting(getQueueName(),
addressSettings);
+ }
@Override
protected void addConfiguration(ActiveMQServer server) {
Configuration serverConfig = server.getConfiguration();
serverConfig.setDiskScanPeriod(100);
}
- @Test
+ @TestTemplate
public void testProducerOnDiskFull() throws Exception {
- FileStoreMonitor monitor =
((ActiveMQServerImpl)server).getMonitor().setMaxUsage(0.0);
- final CountDownLatch latch = new CountDownLatch(1);
+
+ FileStoreMonitor monitor = ((ActiveMQServerImpl)server).getMonitor();
+
+ AtomicBoolean diskUsageOk = new AtomicBoolean(true);
+ AtomicInteger checkValid = new AtomicInteger(0);
+
monitor.addCallback((usableSpace, totalSpace, ok, type) -> {
- latch.countDown();
+
+ if (checkValid.get() == -1) {
+ return;
+ }
+
+ checkValid.incrementAndGet();
+
+ double usage = FileStoreMonitor.calculateUsage(usableSpace,
totalSpace);
+
+ if (type == FileStoreMonitor.FileStoreMonitorType.MaxDiskUsage && ok
&& usage < monitor.getMaxUsage()) {
+ diskUsageOk.set(true);
+ } else if (type == FileStoreMonitor.FileStoreMonitorType.MaxDiskUsage
&& !ok && usage >= monitor.getMaxUsage()) {
+ diskUsageOk.set(false);
+ } else {
+ logger.warn("invalid state, usableSpace: {}, totalSpace: {}, ok:
{}, type: {}", usableSpace, totalSpace, ok, type);
+ checkValid.set(-1);
+ }
});
- assertTrue(latch.await(1, TimeUnit.MINUTES));
+ Wait.assertTrue(() -> checkValid.get() > 0, 1000);
+ Wait.assertTrue(() -> diskUsageOk.get(), 1000);
Review Comment:
Another benefit of switching back to simple latch inspection, most of these
separate waits go and we dont burn any more time than strictly needed (i.e just
the time the monitor callback takes to fire, no additional sleep intervals from
the Wait usage on top).
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/GlobalDiskFullTest.java:
##########
@@ -19,38 +19,99 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.lang.invoke.MethodHandles;
import java.net.URI;
+import java.util.Arrays;
+import java.util.Collection;
import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.activemq.artemis.core.config.Configuration;
import org.apache.activemq.artemis.core.server.ActiveMQServer;
import org.apache.activemq.artemis.core.server.files.FileStoreMonitor;
import org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl;
+import org.apache.activemq.artemis.core.settings.impl.AddressFullMessagePolicy;
+import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
+import org.apache.activemq.artemis.tests.extensions.parameterized.Parameter;
+import
org.apache.activemq.artemis.tests.extensions.parameterized.ParameterizedTestExtension;
+import org.apache.activemq.artemis.tests.extensions.parameterized.Parameters;
+import org.apache.activemq.artemis.utils.Wait;
import org.apache.activemq.transport.amqp.client.AmqpClient;
import org.apache.activemq.transport.amqp.client.AmqpConnection;
import org.apache.activemq.transport.amqp.client.AmqpMessage;
import org.apache.activemq.transport.amqp.client.AmqpSender;
import org.apache.activemq.transport.amqp.client.AmqpSession;
-import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@ExtendWith(ParameterizedTestExtension.class)
public class GlobalDiskFullTest extends AmqpClientTestSupport {
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ @Parameter(index = 0)
+ public AddressFullMessagePolicy addressFullPolicy;
+
+ @Parameters(name = "addressFullPolicy={0}")
+ public static Collection<Object[]> parameters() {
+ return Arrays.asList(new Object[][] {
+ {AddressFullMessagePolicy.FAIL}, {AddressFullMessagePolicy.DROP},
{AddressFullMessagePolicy.PAGE}
+ });
+ }
+
+ @Override
+ protected void configureAddressPolicy(ActiveMQServer server) {
+ AddressSettings addressSettings = new AddressSettings();
+ addressSettings.setAddressFullMessagePolicy(addressFullPolicy);
+ server.getConfiguration().addAddressSetting(getQueueName(),
addressSettings);
+ }
@Override
protected void addConfiguration(ActiveMQServer server) {
Configuration serverConfig = server.getConfiguration();
serverConfig.setDiskScanPeriod(100);
}
- @Test
+ @TestTemplate
public void testProducerOnDiskFull() throws Exception {
- FileStoreMonitor monitor =
((ActiveMQServerImpl)server).getMonitor().setMaxUsage(0.0);
- final CountDownLatch latch = new CountDownLatch(1);
+
+ FileStoreMonitor monitor = ((ActiveMQServerImpl)server).getMonitor();
+
+ AtomicBoolean diskUsageOk = new AtomicBoolean(true);
+ AtomicInteger checkValid = new AtomicInteger(0);
+
monitor.addCallback((usableSpace, totalSpace, ok, type) -> {
- latch.countDown();
+
+ if (checkValid.get() == -1) {
+ return;
+ }
+
+ checkValid.incrementAndGet();
+
+ double usage = FileStoreMonitor.calculateUsage(usableSpace,
totalSpace);
+
+ if (type == FileStoreMonitor.FileStoreMonitorType.MaxDiskUsage && ok
&& usage < monitor.getMaxUsage()) {
+ diskUsageOk.set(true);
+ } else if (type == FileStoreMonitor.FileStoreMonitorType.MaxDiskUsage
&& !ok && usage >= monitor.getMaxUsage()) {
+ diskUsageOk.set(false);
+ } else {
+ logger.warn("invalid state, usableSpace: {}, totalSpace: {}, ok:
{}, type: {}", usableSpace, totalSpace, ok, type);
+ checkValid.set(-1);
+ }
Review Comment:
This seems overly complex and a bit fragile in terms of the asserting, with
its multiple independent values and the different threads in use probably
making asserts span callback firings.
There can be multiple callbacks added. Instead of trying to make one complex
callback work, it would probably be better and clearer to add specific trivial
callbacks for each stage check at the appropriate point, which only trips its
latch when the desired state is reached. You can then assert that the desired
state has been reached (or not) while waiting on the latch after provoking the
state change, but also if desired beforehand assert that it has not been
reached yet by checking the latch doesn't trip yet. Then before the next stage,
add a new callback to verify that new stage.
Eg. add one immediately (essentially the same as the original test (but its
latch gated so it only trips if the state is usage-full), then use it to verify
the not-blocked and blocked state before and after setting the maxUsage to 0.
Once that is done add a new callback with its own latch, do the sends, inspect
latch to verify state is still blocked (not tripped), then after setting the
max to 100 verify that it becomes unblocked (tripped).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
