You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by RaiSaurabh <gi...@git.apache.org> on 2017/12/04 09:22:33 UTC
[GitHub] activemq-artemis pull request #1684: ARTEMIS-1498: Openwire internal headers...
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 <ra...@hotmail.com>
Date: 2017-12-04T09:20:26Z
ARTEMIS-1498: Openwire internal headers should not be part of message properties
----
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by michaelandrepearce <gi...@git.apache.org>.
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();
}
```
---
[GitHub] activemq-artemis pull request #1684: ARTEMIS-1498: Openwire internal headers...
Posted by RaiSaurabh <gi...@git.apache.org>.
Github user RaiSaurabh closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1684
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by michaelandrepearce <gi...@git.apache.org>.
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)
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by RaiSaurabh <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by michaelandrepearce <gi...@git.apache.org>.
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?
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by RaiSaurabh <gi...@git.apache.org>.
Github user RaiSaurabh commented on the issue:
https://github.com/apache/activemq-artemis/pull/1684
@michaelandrepearce Hi Mike could you please review the changes.
---
[GitHub] activemq-artemis pull request #1684: ARTEMIS-1498: Openwire internal headers...
Posted by clebertsuconic <gi...@git.apache.org>.
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
---
[GitHub] activemq-artemis pull request #1684: ARTEMIS-1498: Openwire internal headers...
Posted by jdanekrh <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis pull request #1684: ARTEMIS-1498: Openwire internal headers...
Posted by michaelandrepearce <gi...@git.apache.org>.
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
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by clebertsuconic <gi...@git.apache.org>.
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?
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by RaiSaurabh <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by michaelandrepearce <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by michaelandrepearce <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by RaiSaurabh <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by michaelandrepearce <gi...@git.apache.org>.
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;
}
.....
}
```
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by RaiSaurabh <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by michaelandrepearce <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1684: ARTEMIS-1498: Openwire internal headers should...
Posted by RaiSaurabh <gi...@git.apache.org>.
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.
---