gemmellr commented on code in PR #5430:
URL: https://github.com/apache/activemq-artemis/pull/5430#discussion_r1910026512
##########
tests/activemq5-unit-tests/src/test/java/org/apache/activemq/RedeliveryPolicyTest.java:
##########
@@ -37,6 +37,7 @@
import org.apache.activemq.command.ActiveMQTextMessage;
import org.apache.activemq.command.ActiveMQTopic;
import org.apache.activemq.util.Wait;
+import org.junit.Assert;
Review Comment:
Seems odd to add an import whilst only deleting, is this needed?
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/ClusterTestBase.java:
##########
@@ -1335,6 +1327,15 @@ protected void
verifyReceiveRoundRobinInSomeOrderWithCounts(final boolean ack,
}
+ private static boolean matchCounters(int[] messageCounts, Set<Integer>
counts) {
+ for (int messageCount : messageCounts) {
+ if (!counts.contains(messageCount)) {
+ return false;
+ }
+ }
+ return true;
Review Comment:
This will do nothing (and in turn, so will the calling method) if the passed
in _messageCounts_ doesnt have anything in it, which seems possible looking at
some of the usages. If its expected it always should have something to check,
perhaps an assertion that there is content to be verified?
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/MessageRedistributionTest.java:
##########
@@ -573,7 +570,7 @@ public void testRedistributeWithScheduling() throws
Exception {
ClientProducer prod0 = session0.createProducer("queues.testaddress");
- for (int i = 0; i < 100; i++) {
+ for (int i = 0; i < 10; i++) {
Review Comment:
Adding a NUMBER_OF_MESSAGES constant like the earlier test would probably be
nicer than changing all the related lines to this new literal.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/LargeMessageRedistributionTest.java:
##########
@@ -28,9 +28,51 @@ public boolean isLargeMessage() {
return true;
}
+ @Test
+ @Disabled
+ @Override
+ public void testBackAndForth2WithDuplicDetection() throws Exception {
+ }
+
+ @Test
+ @Disabled
+ @Override
+ public void testRedistributionWhenConsumerIsClosedQueuesWithFilters()
throws Exception {
+ }
+
+ @Test
+ @Disabled
+ @Override
+ public void testRedistributionWithMessageGroups() throws Exception {
+ }
+
+ @Test
+ @Disabled
+ @Override
+ public void testRedistributionWhenConsumerIsClosed() throws Exception {
+ }
+
+ @Test
+ @Disabled
+ @Override
+ public void testRedistributionWithPagingOnTarget() throws Exception {
+ }
+
+ @Test
+ @Disabled
+ @Override
+ public void testBackAndForth2() throws Exception {
+ }
+
+ @Test
+ @Disabled
+ @Override
+ public void testBackAndForth() throws Exception {
+ }
Review Comment:
Some indication why these are being disabled would be good. Or better, if
there are so many that need disabled like this, perhaps a tweak of hierarchy is
more appropriate so that they dont need any override+disable?
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/MessageRedistributionTest.java:
##########
@@ -1297,7 +1281,7 @@ public void testDelayedRedistribution() throws Exception {
@Test
public void testDelayedRedistributionCancelled() throws Exception {
- final long delay = 1000;
+ final long delay = 200;
Review Comment:
I'd have to wonder if this isnt now aggressive enough that it might lead to
more sporadic failures in lower resourced envs (e.g distribution occurs
_before_ it gets cancelled)?
##########
tests/artemis-test-support/src/main/java/org/apache/activemq/artemis/tests/util/ActiveMQTestBase.java:
##########
@@ -545,7 +545,7 @@ protected static final ClusterConnectionConfiguration
basicClusterConnectionConf
setName("cluster1").setAddress("jms").setConnectorName(connectorName).
setRetryInterval(100).setDuplicateDetection(false).setMaxHops(1).
setConfirmationWindowSize(1).setMessageLoadBalancingType(MessageLoadBalancingType.STRICT).
- setStaticConnectors(connectors0);
+
setStaticConnectors(connectors0).setCallTimeout(1000).setCallFailoverTimeout(1000);
Review Comment:
I wonder about this, it feels slightly questionable to apply it to every
test that ever uses this base configuration method. Are there specific tests
that it particularly benefited from it that it could be applied to more
specifically, so the rest can retain the more typical default config?
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/distribution/TwoWayTwoNodeClusterTest.java:
##########
@@ -153,9 +146,8 @@ private void configureBeforeStart(Configuration...
serverConfigs) {
for (Configuration config : serverConfigs) {
config.setPersistenceEnabled(true);
config.setMessageCounterEnabled(true);
- config.setJournalFileSize(20971520);
- config.setJournalMinFiles(20);
- config.setJournalCompactPercentage(50);
+ config.setJournalFileSize(20 * 1024);
Review Comment:
Seems a bit odd it was 20MB before, but only 20KB now? Is there any
indication why was the test was significantly _increasing_ the default value
before, seems odd if its fine now its being made tiny compared to the default?
--
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
