[GitHub] activemq-artemis pull request #1684: ARTEMIS-1498: Openwire internal headers...

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1684: ARTEMIS-1498: Openwire internal headers...

franz1981
GitHub user RaiSaurabh opened a pull request:

    https://github.com/apache/activemq-artemis/pull/1684

    ARTEMIS-1498: Openwire internal headers should not be part of message…

    … properties

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/RaiSaurabh/activemq-artemis header

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/1684.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1684
   
----
commit 563df5d0563d280c2657a4e788a22d980c31af9c
Author: saurabhrai <[hidden email]>
Date:   2017-12-04T09:20:26Z

    ARTEMIS-1498: Openwire internal headers should not be part of message properties

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1684: ARTEMIS-1498: Openwire internal headers...

franz1981
Github user jdanekrh commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1684#discussion_r154599253
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java ---
    @@ -95,7 +95,7 @@
     public class CoreAmqpConverter {
     
        private static Logger logger = Logger.getLogger(CoreAmqpConverter.class);
    -
    +   public static final String INTERNAL_HEADER = "__HDR_";
    --- End diff --
   
    I am thinking whether this variable should have a broader scope and be consistently used instead of the string constant. I know of at least one other such place, the `__HDR_` stripping code in https://github.com/apache/activemq-artemis/pull/1320/files.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    If this is specific to OpenWire headers, should this not be stripped during converting open wire messages within the open wire protocol support module, it shouldn't be leaking into other protocols.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1684: ARTEMIS-1498: Openwire internal headers...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1684#discussion_r154642682
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java ---
    @@ -95,7 +95,7 @@
     public class CoreAmqpConverter {
     
        private static Logger logger = Logger.getLogger(CoreAmqpConverter.class);
    -
    +   public static final String INTERNAL_HEADER = "__HDR_";
    --- End diff --
   
    This should be within the OpenWire protocol module and code, should avoid protocols specifics leaking into other protocol modules / code area's


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1684: ARTEMIS-1498: Openwire internal headers...

franz1981
In reply to this post by franz1981
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1684#discussion_r154658085
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java ---
    @@ -95,7 +95,7 @@
     public class CoreAmqpConverter {
     
        private static Logger logger = Logger.getLogger(CoreAmqpConverter.class);
    -
    +   public static final String INTERNAL_HEADER = "__HDR_";
    --- End diff --
   
    +1


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user RaiSaurabh commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    @michaelandrepearce @clebertsuconic
    Please correct me if my understanding is wrong. I checked the code of OpenwireMesageConverter when we send a message using client if comes to (https://github.com/apache/activemq-artemis/blob/master/artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireMessageConverter.java#L122) where we set all the internal headers with __HDR as message property and return the CoreMessage.
    If we try to receive the message from the Openwire client we use these message properties to set them as ActiveMQMessage object properties like ID etc. Now with the case, if I use the AMQP/ Core receiver the message that is fetched is in form of ICoreMessage in CoreAmqpConverter class (https://github.com/apache/activemq-artemis/blob/master/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java#L104) and all the internal headers of Openwire are present as message properties. Hence I decided to stip it off from the CoreAmqpConverter class. I was not able to find any better place. If you could point me that would be great.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    So here I would either suggest that these are not copied to core message when converting to core, but if need because of consuming with openwire consumer then probably better but a bit more work is to make OpenWire Message so it doesn’t convert on produce only on consume eg some work is done to make it implement some internal interfaces as like what was for for amqp, this would save conversion to core also if producer and consumer are openwire making it more performant.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    On that note, going through the OpenWireConverter that currently converts OpenWire to Core on produce, there is many places where its not setting the correct matching properties (or methods) on core because it has them hardcoded as string, instead of using the constants from Message (in core).
   
    Example
   
    ```
          String groupId = messageSend.getGroupID();
          if (groupId != null) {
             coreMessage.putStringProperty(AMQ_MSG_GROUP_ID, groupId);
          }
    ```
   
    Here the property being set is "__HDR_GROUP_ID" where as in core Message if using the constants from there, then actually what should be set is
   
    ```
          String groupId = messageSend.getGroupID();
          if (groupId != null) {
             coreMessage.putStringProperty(org.apache.activemq.artemis.api.core.Message.HDR_GROUP_ID, SimpleString.toSimpleString(groupId));
          }
    ```
   
    so that it actually sets the correct property than then is handled by other converters properly. (its seems this is quite numerous in the converter, just briefly going through it. )
   
    (please be aware this are of code I'm less familiar in artemis with but a brief look through, i see such oddities)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    @RaiSaurabh seems you've reverted this, are you working on an alternative solution? Is it worth closing this PR till ready?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user RaiSaurabh commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    @michaelandrepearce I have taken your suggestion and implemented it. Currently, I am testing it will push it in a day or two. I am ensuring that all the headers are retained on conversion of protocols.  Hope it is alright for you.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user RaiSaurabh commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    @michaelandrepearce Hi Mike could you please review the changes.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    @RaiSaurabh this isn't what was quite meant, the idea of implementing Message is to avoid conversion, so it only needs to convert if cross protocol on consume. You have only extended and kept the conversion to CoreMessage, not entirely the intent expected.
   
    E.g. the idea is that you have similar to what occurs with AMQPMessage, that internally it is kept unconverted in AMQP form, so that if AMQP producer and AMQP consumer, it doesn't convert to CoreMessage.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    e.g.
   
    ```
    public class OpenwireMessage extends RefCountMessage {
   
       org.apache.activemq.command.Message message;
       
       public OpenwireMessage(org.apache.activemq.command.Message message){
          this.message = message;
       }
   
       @Override
       public SimpleString getReplyTo() {
          return SimpleString.toSimpleString(message.getReplyTo().getPhysicalName());
       }
   
       @Override
       public Message setReplyTo(SimpleString address) {
          message.setReplyTo(ActiveMQDestination.createDestination(address.toString(), ActiveMQDestination.QUEUE_TYPE));
          return this;
       }
   
       @Override
       public Object getUserID() {
          return message.getUserID();
       }
   
       @Override
       public Message setUserID(Object userID) {
          message.setUserID(userID.toString());
          return this;
       }
   
       @Override
       public boolean isDurable() {
          return message.isPersistent();
       }
   
       @Override
       public Message setDurable(boolean durable) {
          message.setPersistent(durable);
          return this;
       }
       .....
    }
    ```


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user RaiSaurabh commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    @michaelandrepearce Thanks, I got the context now will try to update the code and test.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    @RaiSaurabh do you mind rebasing and fixing the conflict please?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user RaiSaurabh commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    @clebertsuconic There seems to be some issue with my branch will close this PR and open a new one.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1684: ARTEMIS-1498: Openwire internal headers...

franz1981
In reply to this post by franz1981
Github user RaiSaurabh closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/1684


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user RaiSaurabh commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    @michaelandrepearce Whatever changes you suggested works fine but the issue that you mentioned that OpenWire specific properties are lost on the restart as won't be encoded down to disk. How can I ensure that  org.apache.activemq.command.Message is encoded down to the disk. This message is only retained if I do not restart the broker.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1684
 
    @RaiSaurabh
   
    look at example methods from
    org.apache.activemq.artemis.protocol.amqp.broker.AMQPMessage
   
    I'd image you'd want to encode and decode the org.apache.activemq.command.Message during persistence and reload.
   
    ```
       @Override
       public int getPersistSize() {
          checkBuffer();
          return DataConstants.SIZE_INT + internalPersistSize();
       }
   
       private int internalPersistSize() {
          return data.array().length;
       }
   
       @Override
       public void persist(ActiveMQBuffer targetRecord) {
          checkBuffer();
          targetRecord.writeInt(internalPersistSize());
          targetRecord.writeBytes(data.array(), 0, data.array().length );
       }
   
       @Override
       public void reloadPersistence(ActiveMQBuffer record) {
          int size = record.readInt();
          byte[] recordArray = new byte[size];
          record.readBytes(recordArray);
          this.messagePaylodStart = 0; // whatever was persisted will be sent
          this.data = Unpooled.wrappedBuffer(recordArray);
          this.bufferValid = true;
          this.durable = true; // it's coming from the journal, so it's durable
          parseHeaders();
       }
    ```


---