You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/03/23 17:02:05 UTC
[GitHub] [activemq-artemis] fvaleri opened a new pull request #3043:
NO-JIRA: OpenWireMessageConverter refactoring
fvaleri opened a new pull request #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043
This is just a refactoring of the OpenWireMessageConverter with no semantics change and is coming out of #2954. Proposed change is similar to what we already have in the AMQP protocol module with a singleton class as the protocol converter entry point and two dedicated classes for conversion algorithms.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] fvaleri commented on issue #3043: ARTEMIS-2682:
OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
fvaleri commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-611066861
Hi the build failure seems to be related to a Travis environment issue because I can run the same build command without issues.
```log
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
```
@clebertsuconic this is what I've done:
- removed `OpenwireMessage` bean because it was just an empty implementation and no other class was using it
- given that the inbound and outbound conversion logic was already distinct inside the `OpenWireMessageConverter` class, I made it as Singleton that is now calling `OpenWireMessageConverter.createInboundMessage` and `CoreOpenWireConverter.createMessageDispatch`
- the conversion logic inside previous methods is exactly the same as it was, I just removed some minor code duplication (*)
- created `OpenWireMessageSupport` class to host some simple utility method and to group all property constants that are in common between `OpenWireMessageConverter` and `CoreOpenWireConverter`. This class also useful in unit tests.
(*) you can easily see my changes by comparing the following:
- `OpenWireMessageConverter.inbound` --> `OpenWireCoreConverter.createInboundMessage`
- `OpenWireMessageConverter.createMessageDispatch` --> `CoreOpenWireConverter.createMessageDispatch`
I know it's just a starter, but I'm really looking forward to your comments. I think the converter is much more manageable than before and, along with unit tests, it should allow us to be more confident in case of future changes.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain edited a comment on issue
#3043: NO-JIRA: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain edited a comment on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-604126241
fyi, because of the amounts of changed lines, annoying it hasnt been done so it shows it as a move, and just the edited lines, this is taking some time to go through.
Also whats the need here to do such refactor, if there is no change....isnt it a change that brings no feature of fix improvement, but just adds risk of accidental introduced bug...... im coming from a stability pov (which we keep suffering)
To that note, this seemless innocent refactor doesnt actually compiile.... and thus my concern about such changes....
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on issue #3043:
NO-JIRA: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-602924170
@michaelandrepearce I talked to @fvaleri and I had told him that if he's sure this is a no-semantic change we could have a NO-JIRA as being just a reorg on the code.
but if you see a need we can certainly issue a JIRA.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-610629071
This still isnt compiling.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #3043:
NO-JIRA: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-604295641
The main point is this needs a jira. Its not a small change
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] fvaleri commented on issue #3043: ARTEMIS-2682:
OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
fvaleri commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-613270226
OK, changed Bin to Binary and rebased.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain edited a comment on issue
#3043: NO-JIRA: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain edited a comment on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-604126241
fyi, because of the amounts of changed lines, annoying it hasnt been done so it shows it as a move, and just the edited lines, this is taking some time to go through.
Also whats the need here to do such refactor, if there is no change....isnt it a change that brings no feature of fix improvement, but just adds risk of accidental introduced bug...... im coming from a stability pov (which we keep suffering)
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain edited a comment on issue
#3043: NO-JIRA: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain edited a comment on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-604126241
fyi, because of the amounts of changed lines, annoying it hasnt been done so it shows it as a move, and just the edited lines, this is taking some time to go through.
Also whats the need here to do such refactor, if there is no change....isnt it a change that brings no feature of fix improvement, but just adds risk of accidental introduced bug......
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-610642806
These tests are good BTW. we should include them some way or the other.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-605369255
its still not compiling.....even after your latest commit, i really am thinking is this PR worth it, if its only a refactor, but not benefit.
The biggest thing for me in this space if someone was to refactor is to actually introduce Native OpenWireMessage support, so for openwire to openwire we dont convert. Then it be worth the risk/effort.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-610644528
@clebertsuconic i keep getting a compile error with this pr due to a classname not matching its file.
I will try full rebase tomorrow if given time. That said i havent even started to review this as need to have it compile first.
Also with all the basic compile issues, and the amount of change which makes it hard to work out what actually had changed, this is a high risk pr that clearly can introduce an accidental issue (as example it hasnt been compiling)
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on issue #3043:
NO-JIRA: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-604125858
Its not a small change tho, lets have a JIRA. If it was just a move of files or something, then it show as a git move, but it seems that its wholesale class changes...with that we should be careful as its very hard to cross validate before and after.... in git diff, having to manually review this currently.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] fvaleri commented on a change in pull request
#3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
fvaleri commented on a change in pull request #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#discussion_r407913973
##########
File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireMessageSupport.java
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.protocol.openwire;
+
+import java.io.IOException;
+
+import org.apache.activemq.artemis.api.core.ICoreMessage;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.util.ByteSequence;
+import org.apache.activemq.wireformat.WireFormat;
+
+public class OpenWireMessageSupport {
+
+ public static final SimpleString JMS_TYPE_PROPERTY = new SimpleString("JMSType");
+ public static final SimpleString JMS_CORRELATION_ID_PROPERTY = new SimpleString("JMSCorrelationID");
+ public static final SimpleString AMQ_PREFIX = new SimpleString("__HDR_");
+
+ public static final SimpleString AMQ_MSG_DLQ_DELIVERY_FAILURE_CAUSE_PROPERTY = new SimpleString(AMQ_PREFIX + "dlqDeliveryFailureCause");
+ public static final SimpleString AMQ_MSG_ARRIVAL = new SimpleString(AMQ_PREFIX + "ARRIVAL");
+ public static final SimpleString AMQ_MSG_BROKER_IN_TIME = new SimpleString(AMQ_PREFIX + "BROKER_IN_TIME");
+ public static final SimpleString AMQ_MSG_BROKER_PATH = new SimpleString(AMQ_PREFIX + "BROKER_PATH");
+ public static final SimpleString AMQ_MSG_CLUSTER = new SimpleString(AMQ_PREFIX + "CLUSTER");
+ public static final SimpleString AMQ_MSG_COMMAND_ID = new SimpleString(AMQ_PREFIX + "COMMAND_ID");
+ public static final SimpleString AMQ_MSG_DATASTRUCTURE = new SimpleString(AMQ_PREFIX + "DATASTRUCTURE");
+ public static final SimpleString AMQ_MSG_MESSAGE_ID = new SimpleString(AMQ_PREFIX + "MESSAGE_ID");
+ public static final SimpleString AMQ_MSG_ORIG_DESTINATION = new SimpleString(AMQ_PREFIX + "ORIG_DESTINATION");
+ public static final SimpleString AMQ_MSG_ORIG_TXID = new SimpleString(AMQ_PREFIX + "ORIG_TXID");
+ public static final SimpleString AMQ_MSG_PRODUCER_ID = new SimpleString(AMQ_PREFIX + "PRODUCER_ID");
+ public static final SimpleString AMQ_MSG_MARSHALL_PROP = new SimpleString(AMQ_PREFIX + "MARSHALL_PROP");
+ public static final SimpleString AMQ_MSG_REPLY_TO = new SimpleString(AMQ_PREFIX + "REPLY_TO");
+ public static final SimpleString AMQ_MSG_USER_ID = new SimpleString(AMQ_PREFIX + "USER_ID");
+ public static final SimpleString AMQ_MSG_DROPPABLE = new SimpleString(AMQ_PREFIX + "DROPPABLE");
+ public static final SimpleString AMQ_MSG_COMPRESSED = new SimpleString(AMQ_PREFIX + "COMPRESSED");
+
+ public static Object getBinObjectProperty(WireFormat format,
+ ICoreMessage coreMessage,
+ SimpleString propertyKey) throws IOException {
+ if (format == null || coreMessage == null || isNullOrEmpty(propertyKey)) {
+ return null;
+ }
+ Object object = coreMessage.getObjectProperty(propertyKey);
+ if (object == null || !(object instanceof byte[])) {
+ return null;
+ }
+ ByteSequence seq = new ByteSequence((byte[]) object);
+ return format.unmarshal(seq);
+ }
+
+ public static void setBinObjectProperty(WireFormat format,
Review comment:
Sure
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-613088847
can you rebase this?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] fvaleri edited a comment on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
fvaleri edited a comment on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-611066861
Hi the build failure seems to be related to a Travis environment issue. I can successfully run the same build command without issues.
```log
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
```
@clebertsuconic this is what I've done:
- removed `OpenwireMessage` bean because it was just an empty implementation and no other class was using it
- given that the inbound and outbound conversion logic was already distinct inside the `OpenWireMessageConverter` class, I made it as Singleton that is now calling `OpenWireMessageConverter.createInboundMessage` and `CoreOpenWireConverter.createMessageDispatch`
- the conversion logic inside previous methods is exactly the same as it was, I just removed some minor code duplication (*)
- created `OpenWireMessageSupport` class to host some simple utility methods and to group all property constants that are in common between `OpenWireMessageConverter` and `CoreOpenWireConverter`. This class also useful in unit tests.
(*) you can easily see my changes by comparing the following:
- `OpenWireMessageConverter.inbound` (master) --> `OpenWireCoreConverter.createInboundMessage` (branch)
- `OpenWireMessageConverter.createMessageDispatch` (master) --> `CoreOpenWireConverter.createMessageDispatch` (branch)
I know it's just a starter, but I'm really looking forward to your comments. I think the converter is much more manageable than before and, along with unit tests, it should allow us to be more confident in case of future changes.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain edited a comment on issue
#3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain edited a comment on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-605369255
its still not compiling.....even after your latest commit, i really am thinking is this PR worth it, if its only a refactor, but no benefit, just risk of what else is broken, if it doesnt even compile atm...?!?!
The biggest thing for me in this space if someone was to refactor is to actually introduce Native OpenWireMessage support, so for openwire to openwire we dont convert. Then it be worth the risk/effort.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] fvaleri commented on issue #3043: ARTEMIS-2682:
OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
fvaleri commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-605665524
Same reason as before, I hope the fix is pushed now and I also added an initial set of unit tests.
I think that a feature like native OpenWire message support would inevitably include some refactoring. If you are not sensible to better maintainability, then you could also take this as a first step in that direction.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#discussion_r407740665
##########
File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireMessageSupport.java
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.protocol.openwire;
+
+import java.io.IOException;
+
+import org.apache.activemq.artemis.api.core.ICoreMessage;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.util.ByteSequence;
+import org.apache.activemq.wireformat.WireFormat;
+
+public class OpenWireMessageSupport {
+
+ public static final SimpleString JMS_TYPE_PROPERTY = new SimpleString("JMSType");
+ public static final SimpleString JMS_CORRELATION_ID_PROPERTY = new SimpleString("JMSCorrelationID");
+ public static final SimpleString AMQ_PREFIX = new SimpleString("__HDR_");
+
+ public static final SimpleString AMQ_MSG_DLQ_DELIVERY_FAILURE_CAUSE_PROPERTY = new SimpleString(AMQ_PREFIX + "dlqDeliveryFailureCause");
+ public static final SimpleString AMQ_MSG_ARRIVAL = new SimpleString(AMQ_PREFIX + "ARRIVAL");
+ public static final SimpleString AMQ_MSG_BROKER_IN_TIME = new SimpleString(AMQ_PREFIX + "BROKER_IN_TIME");
+ public static final SimpleString AMQ_MSG_BROKER_PATH = new SimpleString(AMQ_PREFIX + "BROKER_PATH");
+ public static final SimpleString AMQ_MSG_CLUSTER = new SimpleString(AMQ_PREFIX + "CLUSTER");
+ public static final SimpleString AMQ_MSG_COMMAND_ID = new SimpleString(AMQ_PREFIX + "COMMAND_ID");
+ public static final SimpleString AMQ_MSG_DATASTRUCTURE = new SimpleString(AMQ_PREFIX + "DATASTRUCTURE");
+ public static final SimpleString AMQ_MSG_MESSAGE_ID = new SimpleString(AMQ_PREFIX + "MESSAGE_ID");
+ public static final SimpleString AMQ_MSG_ORIG_DESTINATION = new SimpleString(AMQ_PREFIX + "ORIG_DESTINATION");
+ public static final SimpleString AMQ_MSG_ORIG_TXID = new SimpleString(AMQ_PREFIX + "ORIG_TXID");
+ public static final SimpleString AMQ_MSG_PRODUCER_ID = new SimpleString(AMQ_PREFIX + "PRODUCER_ID");
+ public static final SimpleString AMQ_MSG_MARSHALL_PROP = new SimpleString(AMQ_PREFIX + "MARSHALL_PROP");
+ public static final SimpleString AMQ_MSG_REPLY_TO = new SimpleString(AMQ_PREFIX + "REPLY_TO");
+ public static final SimpleString AMQ_MSG_USER_ID = new SimpleString(AMQ_PREFIX + "USER_ID");
+ public static final SimpleString AMQ_MSG_DROPPABLE = new SimpleString(AMQ_PREFIX + "DROPPABLE");
+ public static final SimpleString AMQ_MSG_COMPRESSED = new SimpleString(AMQ_PREFIX + "COMPRESSED");
+
+ public static Object getBinObjectProperty(WireFormat format,
+ ICoreMessage coreMessage,
+ SimpleString propertyKey) throws IOException {
+ if (format == null || coreMessage == null || isNullOrEmpty(propertyKey)) {
+ return null;
+ }
+ Object object = coreMessage.getObjectProperty(propertyKey);
+ if (object == null || !(object instanceof byte[])) {
+ return null;
+ }
+ ByteSequence seq = new ByteSequence((byte[]) object);
+ return format.unmarshal(seq);
+ }
+
+ public static void setBinObjectProperty(WireFormat format,
Review comment:
Bin? Please lets call it binary
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain edited a comment on issue
#3043: NO-JIRA: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain edited a comment on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-604126241
fyi, because of the amounts of changed lines, annoying it hasnt been done so it shows it as a move, and just the edited lines, this is taking some time to go through.
Also whats the need here to do such refactor, if there is no change....
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on issue #3043:
NO-JIRA: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-604126241
fyi, because of the amounts of changed lines, annoying it hasnt been done so it shows it as a move, and just the edited lines, this is taking some time to go through.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] fvaleri edited a comment on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
fvaleri edited a comment on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-611066861
Hi the build failure seems to be related to a Travis environment issue. I can successfully run the same build command without issues.
```log
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
```
@clebertsuconic this is what I've done:
- removed `OpenwireMessage` bean because it was just an empty implementation and no other class was using it
- given that the inbound and outbound conversion logic was already distinct inside the `OpenWireMessageConverter` class, I made it as Singleton that is now calling `OpenWireMessageConverter.createInboundMessage` and `CoreOpenWireConverter.createMessageDispatch`
- the conversion logic inside previous methods is exactly the same as it was, I just removed some minor code duplication (*)
- created `OpenWireMessageSupport` class to host some simple utility methods and to group all property constants that are in common between `OpenWireMessageConverter` and `CoreOpenWireConverter`. This class also useful in unit tests.
(*) you can easily see my changes by comparing the following:
- `OpenWireMessageConverter.inbound` --> `OpenWireCoreConverter.createInboundMessage`
- `OpenWireMessageConverter.createMessageDispatch` --> `CoreOpenWireConverter.createMessageDispatch`
I know it's just a starter, but I'm really looking forward to your comments. I think the converter is much more manageable than before and, along with unit tests, it should allow us to be more confident in case of future changes.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce edited a comment on issue
#3043: NO-JIRA: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelandrepearce edited a comment on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-602786096
This requires a jira that clearly states why such a large change in the code base without any fix or improvement such as native openwire message support
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic edited a comment on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
clebertsuconic edited a comment on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-610639997
@michaelpearce-gain I actually rebased this locally and it's building fine. it's probably some intermittent issue on the CI.
@fvaleri Can you rebase it and push -f?
can you describe what you have moved here?
i'm trying to understand the gain of these changes.
I remember when i wrote this I couldn't put the OpenWire in the same classModel I did for AMQP. Is that what you accomplished here?
if so.. what exactly did you move? it's kind of hard to see if there was no changes in here. and the benefit of such change.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-610639997
@michaelpearce-gain I actually rebased this locally and it's building fine. it's probably some intermittent issue on the CI.
Can you rebase it and push -f?
can you describe what you have moved here?
i'm trying to understand the gain of these changes.
I remember when i wrote this I couldn't put the OpenWire in the same classModel I did for AMQP. Is that what you accomplished here?
if so.. what exactly did you move? it's kind of hard to see if there was no changes in here. and the benefit of such change.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #3043:
NO-JIRA: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-602786096
This requires a jira
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] fvaleri commented on issue #3043: NO-JIRA:
OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
fvaleri commented on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-604290495
@michaelpearce-gain there was a small error in a class rename, now fixed.
I completely understand that stability matter the most and it may take some time to be verified by another developer. I'm also open to discussion here.
The whole point of refactoring is to alter the internal structure without changing its external behavior, so there shouldn't be new features or fixes. The reward in this case should be better maintainability, at least this is the intention.
Specifically, this is copying the AMQP protocol module layout, which splits the inbound and outbound converters in dedicated classes (they were already distinct but hosted on the same big class). In my opinion this is much more clean.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-610676697
@michaelandrepearce try `git clean -df` after you checkout this branch.
I created a branch called forMichael on my fork, if that helps.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-610629216
I really now have reservations on this PR from a stability standpoint
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain edited a comment on issue
#3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain edited a comment on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-605369255
its still not compiling.....even after your latest commit, i really am thinking is this PR worth it, if its only a refactor, but not benefit, just risk oi what else is broken, if it doesnt even compile?!?!
The biggest thing for me in this space if someone was to refactor is to actually introduce Native OpenWireMessage support, so for openwire to openwire we dont convert. Then it be worth the risk/effort.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain edited a comment on issue
#3043: NO-JIRA: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain edited a comment on issue #3043: NO-JIRA: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-604126241
fyi, because of the amounts of changed lines, annoying it hasnt been done so it shows it as a move, and just the edited lines, this is taking some time to go through.
Also whats the need here to do such refactor, if there is no change....isnt it a change that brings no feature of fix improvement, but just adds risk of accidental introduced bug...... im coming from a stability pov (which we keep suffering)
To that note, this seemless innocent refactor doesnt actually compiile.... and thus my concern about such changes bringing risk, but no reward....
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] fvaleri commented on issue #3043: ARTEMIS-2682:
OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
fvaleri commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-604304058
> The main point is this needs a jira. Its not a small change
Agree and now you have it.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #3043:
ARTEMIS-2682: OpenWireMessageConverter refactoring
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on issue #3043: ARTEMIS-2682: OpenWireMessageConverter refactoring
URL: https://github.com/apache/activemq-artemis/pull/3043#issuecomment-613113293
So i managed to get this to compile finally.
I have to say the tests are a nice addition.
That said having a single class (as was) for the conversion to and from core or two seperate is really negligible in difference and is more personal coding preference/ style of a coder.. it makes no difference in ability to maintain.
Im personally much more in favour of a single class containing the conversion logic as it keeps it clearly contained.
----------------------------------------------------------------
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