franz1981 edited a comment on issue #2975: ARTEMIS-2617 Improve AMQP Journal loading URL: https://github.com/apache/activemq-artemis/pull/2975#issuecomment-583875107 @tabish121 @gemmellr @clebertsuconic @michaelandrepearce This PR contains many changes and I know there are parts that could be simplified a lot. The most important bits that can really be a game-changer for AMQP on journal loading are on https://github.com/apache/activemq-artemis/pull/2975/commits/fc77a546ca1ee047218e938f0413c55ed8c838c6. So please review especially that part. An important note: I've found that `AMQPMessagePersisterV2::decode` does something weird re `AMQPMessage::setAddress`: `AMQPMessagePersister::decode` can `AMQPMessage::setAddress`, allocating `AMQPMessage::extraProperties`, but `AMQPMessagePersisterV2::decode` can decode a new `extraProperties` totally overwriting the existing one. Is it a bug or it's suppose to work like that? I see that this behaviour has been introduced on `ARTEMIS-1858` in 1ae2784dc6075875b18780fa8ba40f86cb895f7b with this comment: ```java /** * This will set the address on CoreMessage. * * Note for AMQPMessages: * in AMQPMessages this will not really change the address on the message. Instead it will add a property * on extraProperties which only transverse internally at the broker. * Whatever you change here it won't affect anything towards the received message. * * If you wish to change AMQPMessages address you will have to do it directly at the AMQP Message, however beware * that AMQPMessages are not supposed to be changed at the broker, so only do it if you know what you are doing. * @param address * @return */ Message setAddress(SimpleString address); ``` I've tried to mitigate this by using `CoreMessageObjectPools` to save `SimpleString` allocations [here](https://github.com/franz1981/activemq-artemis/blob/918800b309009fd440bf83b69b54a0d4f49db940/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessagePersisterV2.java#L78) and by allowing `TypedProperties::decode` to append/replace any existing `extraProperty`, saving an `HashMap` allocation, but IMO it could be addressed by properly fixing it, wdyt? any idea how?
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
