gemmellr commented on code in PR #4888:
URL: https://github.com/apache/activemq-artemis/pull/4888#discussion_r1566979146
##########
tests/compatibility-tests/src/test/java/org/apache/activemq/artemis/tests/compatibility/MirroredVersionTest.java:
##########
@@ -106,7 +110,6 @@ public void testMirrorReplicat(int stringSize) throws
Throwable {
logger.debug("Starting live");
evaluate(mainClassloader, "multiVersionMirror/mainServer.groovy",
serverFolder.getRoot().getAbsolutePath(), "1");
logger.debug("Starting backup");
- evaluate(backupClassLoader, "multiVersionMirror/backupServer.groovy",
serverFolder.getRoot().getAbsolutePath(), "2");
Review Comment:
This is being removed, but immediately above its still being logged that it
is being done. The next thing that happens (before or after the sends) is not
starting a backup. Seems like the log should be moved.
##########
tests/compatibility-tests/src/main/resources/multiVersionMirror/mainServer.groovy:
##########
@@ -39,10 +40,11 @@ configuration.setPersistenceEnabled(true);
configuration.addAddressConfiguration(new
CoreAddressConfiguration().setName("TestQueue").addRoutingType(RoutingType.ANYCAST));
configuration.addQueueConfiguration(new
QueueConfiguration("TestQueue").setAddress("TestQueue").setRoutingType(RoutingType.ANYCAST));
+configuration.addAddressConfiguration(new
CoreAddressConfiguration().setName("TestTopic").addRoutingType(RoutingType.MULTICAST));
try {
AMQPBrokerConnectConfiguration connection = new
AMQPBrokerConnectConfiguration("mirror",
"tcp://localhost:61617").setReconnectAttempts(-1).setRetryInterval(100);
- AMQPMirrorBrokerConnectionElement replication = new
AMQPMirrorBrokerConnectionElement().setDurable(true).setSync(true).setMessageAcknowledgements(true);
+ AMQPMirrorBrokerConnectionElement replication = new
AMQPMirrorBrokerConnectionElement().setDurable(true).setSync(false).setMessageAcknowledgements(true).setDurable(true);
Review Comment:
Added .setDurable(true) at end means it is being set twice since it was
already being set true.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java:
##########
@@ -825,8 +826,14 @@ protected ReadableBuffer createDeliveryCopy(int
deliveryCount, DeliveryAnnotatio
}
writeDeliveryAnnotationsForSendBuffer(result, deliveryAnnotations);
- // skip existing delivery annotations of the original message
- duplicate.position(encodedHeaderSize + encodedDeliveryAnnotationsSize);
+
+ if (headerPosition > deliveryAnnotationsPosition && headerPosition !=
VALUE_NOT_PRESENT && deliveryAnnotationsPosition != VALUE_NOT_PRESENT) {
+ // this is for a case where delivery annotations was swiched wrongly
in a previous version
+ duplicate.position(deliveryAnnotationsPosition +
encodedDeliveryAnnotationsSize);
Review Comment:
I dont really understand this. Is it trying to _handle_ the
delivery-annotations having been written in the wrong place, in front of the
header, in an old stored message?
If so, how does setting the position to the end of the delivery annotations
do that? Wouldnt that mean it was now positioned at the start of the
illegally-positioned header...which we have already just written [if needed]
previously a few lines earlier in this method?
##########
tests/compatibility-tests/src/test/java/org/apache/activemq/artemis/tests/compatibility/MirroredVersionTest.java:
##########
@@ -154,4 +161,95 @@ public void testMirrorReplicat(int stringSize) throws
Throwable {
session.commit();
}
}
+
+
+ @Test
+ public void testTopic() throws Throwable {
+ int stringSize = 100;
+ String body = createBody(stringSize);
+ logger.debug("Starting live");
+ evaluate(mainClassloader, "multiVersionMirror/mainServer.groovy",
serverFolder.getRoot().getAbsolutePath(), "1");
+ logger.debug("Starting backup");
+ evaluate(backupClassLoader, "multiVersionMirror/backupServer.groovy",
serverFolder.getRoot().getAbsolutePath(), "2");
+
+ ConnectionFactory factoryMain = new
JmsConnectionFactory("amqp://localhost:61616");
+
+ try (javax.jms.Connection connection = factoryMain.createConnection()) {
+ connection.setClientID("connection1");
+ Session session = connection.createSession(true,
Session.SESSION_TRANSACTED);
+ Topic topic = session.createTopic("TestTopic");
+ MessageConsumer consumer = session.createDurableConsumer(topic,
"Topic1");
Review Comment:
Some constants instead of having things like "TestTopic" and "Topic1"
(Subscription1?) etc repeated in lots of places would make it nicer follow what
the test does where. They could also e.g. be referenced in the 'matching
groovy' for server setup etc and make the link and behaviours between those two
separate bits of code more discoverable.
--
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]
