[
https://issues.apache.org/jira/browse/ARTEMIS-4725?focusedWorklogId=914819&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-914819
]
ASF GitHub Bot logged work on ARTEMIS-4725:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 16/Apr/24 09:40
Start Date: 16/Apr/24 09:40
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 914819)
Time Spent: 0.5h (was: 20m)
> Mirror may send wrong headers
> -----------------------------
>
> Key: ARTEMIS-4725
> URL: https://issues.apache.org/jira/browse/ARTEMIS-4725
> Project: ActiveMQ Artemis
> Issue Type: Bug
> Reporter: Clebert Suconic
> Assignee: Clebert Suconic
> Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> This is not specifically an issue in Mirroring or Broker Connection, but it
> manifested as part of the broker connection codebase.
> When a delivery for the first time after a reload happens, the delivery
> annotation may be written before the header, breaking the specification.
> Later on delivery the message, createDelieryCopy could get confused with the
> positions:
> https://github.com/apache/activemq-artemis/blob/50fae08b09a76e200ef107d06cc867231f644ccd/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java#L829
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
