[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size

[GitBox <gi...@apache.org>]

clebertsuconic commented on a change in pull request #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019#discussion_r393231077
 
 

 ##########
 File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessagePersisterV2.java
 ##########
 @@ -72,25 +75,36 @@ public void encode(ActiveMQBuffer buffer, Message record) {
    }
 
    @Override
-   public Message decode(ActiveMQBuffer buffer, Message record, CoreMessageObjectPools pool) {
-      AMQPMessage message = (AMQPMessage) super.decode(buffer, record, pool);
+   public Message decode(ActiveMQBuffer buffer, Message ignore, CoreMessageObjectPools pool) {
+      // IMPORTANT:
+      // This is a sightly modified copy of the AMQPMessagePersister::decode body
+      // to save extraProperties to be created twice: this would kill GC during journal loading
+      long id = buffer.readLong();
+      long format = buffer.readLong();
+      // this instance is being used only if there are no extraProperties or just for debugging purposes:
+      // on journal loading pool shouldn't be null so it shouldn't create any garbage.
+      final SimpleString address;
+      if (pool == null) {
+         address = buffer.readNullableSimpleString();
+      } else {
+         address = SimpleString.readNullableSimpleString(buffer.byteBuf(), pool.getAddressDecoderPool());
+      }
+      AMQPStandardMessage record = new AMQPStandardMessage(format);
+      record.reloadPersistence(buffer, pool);
+      record.setMessageID(id);
+      // END of AMQPMessagePersister::decode body copy
       int size = buffer.readInt();
-
       if (size != 0) {
-         // message::setAddress could have populated extra properties
-         // hence, we can safely replace the value on the properties
-         // if it has been encoded differently in the rest of the buffer
-         TypedProperties existingExtraProperties = message.getExtraProperties();
-         TypedProperties extraProperties = existingExtraProperties;
-         if (existingExtraProperties == null) {
-            extraProperties = new TypedProperties(Message.INTERNAL_PROPERTY_NAMES_PREDICATE);
-         }
-         extraProperties.decode(buffer.byteBuf(), pool != null ? pool.getPropertiesDecoderPools() : null, existingExtraProperties == null);
-         if (extraProperties != existingExtraProperties) {
-            message.setExtraProperties(extraProperties);
-         }
+         final TypedProperties extraProperties = record.createExtraProperties();
+         extraProperties.decode(buffer.byteBuf(), pool != null ? pool.getPropertiesDecoderPools() : null);
+         assert Objects.equals(address, extraProperties.getSimpleStringProperty(AMQPMessage.ADDRESS_PROPERTY)) :
 
 Review comment:
   Can't we just log.debug if something is wrong here?
   log.warn...
   
   i would rather have a big warning on the system, with the system somewhat working, than having AssertException and having the system freezing.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services