gemmellr commented on code in PR #5173:
URL: https://github.com/apache/activemq-artemis/pull/5173#discussion_r1732610413
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/connect/mirror/AckManager.java:
##########
@@ -77,9 +75,8 @@ public AckManager(ActiveMQServer server) {
this.server = server;
this.configuration = server.getConfiguration();
this.ioCriticalErrorListener = server.getIoCriticalErrorListener();
- this.journal = server.getStorageManager().getMessageJournal();
this.sequenceGenerator = server.getStorageManager()::generateID;
- journalHashMapProvider = new JournalHashMapProvider<>(sequenceGenerator,
journal, AckRetry.getPersister(), JournalRecordIds.ACK_RETRY,
OperationContextImpl::getContext, server.getPostOffice()::findQueue,
server.getIoCriticalErrorListener());
+ journalHashMapProvider = new JournalHashMapProvider<>(sequenceGenerator,
server.getStorageManager(), AckRetry.getPersister(),
JournalRecordIds.ACK_RETRY, OperationContextImpl::getContext,
server.getPostOffice()::findQueue, server.getIoCriticalErrorListener());
Review Comment:
Took me a fair bit to figure out this line is the main fix, with everything
else just 'wrapping' to accommodate making it. Would be good to add some basic
details to the Jira explaining what the issue actually was and how it is being
addressed.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/replication/RepeatStartBackupTest.java:
##########
@@ -156,8 +171,110 @@ public void testLoopStart() throws Exception {
Assertions.assertEquals(0, errors.get());
}
+ }
+
+ @Test
+ public void testAckManagerRepetition() throws Exception {
+
+ String queueName = "queue_" + RandomUtil.randomString();
+
+ server.getConfiguration().setMirrorAckManagerQueueAttempts(300000);
+ server.getConfiguration().setMirrorAckManagerRetryDelay(1000);
+ backupServer.getConfiguration().setMirrorAckManagerPageAttempts(300000);
+ backupServer.getConfiguration().setMirrorAckManagerRetryDelay(1000);
+
+ ExecutorService executorService = Executors.newFixedThreadPool(2);
+ runAfter(executorService::shutdownNow);
+
+ AtomicInteger errors = new AtomicInteger(0);
+ AtomicBoolean running = new AtomicBoolean(true);
+
+ runAfter(() -> running.set(false));
+ CountDownLatch latch = new CountDownLatch(1);
+ CountDownLatch backupStarted = new CountDownLatch(1);
+
+ AtomicInteger messagesSent = new AtomicInteger(0);
Review Comment:
Given no messages are sent this ends up being a bit confusing, perhaps
messageAcks ?
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/replication/RepeatStartBackupTest.java:
##########
@@ -156,8 +171,110 @@ public void testLoopStart() throws Exception {
Assertions.assertEquals(0, errors.get());
}
+ }
+
+ @Test
+ public void testAckManagerRepetition() throws Exception {
+
+ String queueName = "queue_" + RandomUtil.randomString();
+
+ server.getConfiguration().setMirrorAckManagerQueueAttempts(300000);
+ server.getConfiguration().setMirrorAckManagerRetryDelay(1000);
+ backupServer.getConfiguration().setMirrorAckManagerPageAttempts(300000);
+ backupServer.getConfiguration().setMirrorAckManagerRetryDelay(1000);
Review Comment:
Do the delays mean the test basically _has to_ take > 1second? Is that
really needed? Similarly, given the delays, are the massive attempts necessary?
##########
tests/soak-tests/src/test/java/org/apache/activemq/artemis/tests/soak/brokerConnection/mirror/ReplicatedBothNodesMirrorTest.java:
##########
@@ -22,6 +22,7 @@
import javax.jms.JMSException;
import javax.jms.MessageConsumer;
import javax.jms.MessageProducer;
+import javax.jms.Queue;
Review Comment:
General comment about the class, not this specific change.
This test class failed in the CI run you did, perhaps something to look at.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/replication/RepeatStartBackupTest.java:
##########
@@ -156,8 +171,110 @@ public void testLoopStart() throws Exception {
Assertions.assertEquals(0, errors.get());
}
+ }
+
+ @Test
+ public void testAckManagerRepetition() throws Exception {
+
+ String queueName = "queue_" + RandomUtil.randomString();
+
+ server.getConfiguration().setMirrorAckManagerQueueAttempts(300000);
+ server.getConfiguration().setMirrorAckManagerRetryDelay(1000);
+ backupServer.getConfiguration().setMirrorAckManagerPageAttempts(300000);
+ backupServer.getConfiguration().setMirrorAckManagerRetryDelay(1000);
+
+ ExecutorService executorService = Executors.newFixedThreadPool(2);
+ runAfter(executorService::shutdownNow);
+
+ AtomicInteger errors = new AtomicInteger(0);
+ AtomicBoolean running = new AtomicBoolean(true);
+
+ runAfter(() -> running.set(false));
+ CountDownLatch latch = new CountDownLatch(1);
+ CountDownLatch backupStarted = new CountDownLatch(1);
+
+ AtomicInteger messagesSent = new AtomicInteger(0);
+
+ int starBackupAt = 100;
+ Assertions.assertFalse(server.isReplicaSync());
+ Assertions.assertFalse(backupServer.isStarted());
Review Comment:
We can lose the _Assertions._ prefixes (here and elsewhere) with imports,
like the existing ones for other assert methods.
##########
tests/soak-tests/src/test/java/org/apache/activemq/artemis/tests/soak/brokerConnection/mirror/ReplicatedBothNodesMirrorTest.java:
##########
@@ -185,19 +190,25 @@ private static void createMirroredServer(String
serverName,
brokerProperties.put("AMQPConnections." + connectionName +
".connectionElements.mirror.sync", "false");
brokerProperties.put("largeMessageSync", "false");
- brokerProperties.put("addressSettings.#.maxSizeMessages", "50");
- brokerProperties.put("addressSettings.#.maxReadPageMessages", "2000");
- brokerProperties.put("addressSettings.#.maxReadPageBytes", "-1");
- brokerProperties.put("addressSettings.#.prefetchPageMessages", "500");
+ brokerProperties.put("mirrorAckManagerQueueAttempts", "5");
+ brokerProperties.put("mirrorAckManagerPageAttempts", "500000");
+ brokerProperties.put("mirrorAckManagerRetryDelay", "500");
Review Comment:
Same overall feedback the earlier PR [1]. This seems unexpected for the
non-paging case, I don't understand it being this way (or exactly what it is
going to do based on your reply) at present , and its only going to be more
difficult to understand it later. It could do with a comment explaining why the
non-paging test is using paging config and what it is going to do, or changing
to case-specific config given there is already a boolean being used to toggle
the two different cases anyway.
[1]
https://github.com/apache/activemq-artemis/pull/5164#discussion_r1731305285
--
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: gitbox-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
