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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

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

pgfox
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...

pgfox
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...

pgfox
In reply to this post by pgfox
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...

pgfox
In reply to this post by pgfox
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...

pgfox
In reply to this post by pgfox
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...

pgfox
In reply to this post by pgfox
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...

pgfox
In reply to this post by pgfox
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...

pgfox
In reply to this post by pgfox
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...

pgfox
In reply to this post by pgfox
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...

pgfox
In reply to this post by pgfox
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.


---