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 2021/02/10 16:51:59 UTC
[GitHub] [activemq-artemis] ehsavoie opened a new pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
ehsavoie opened a new pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448
* removing the JMS dependency on AMQP module
* fixing destinations usage.
Jira: https://issues.apache.org/jira/browse/ARTEMIS-3113
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574072007
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMapMessage.java
##########
@@ -137,84 +123,75 @@ public void setObject(final String name, final Object value) throws JMSException
}
TypedProperties.setObjectProperty(new SimpleString(name), val, map);
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public boolean getBoolean(final String name) throws JMSException {
+ public boolean getBoolean(final String name) throws Exception {
try {
return map.getBooleanProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public byte getByte(final String name) throws JMSException {
+ public byte getByte(final String name) throws Exception {
try {
return map.getByteProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
Review comment:
Least that way youd get something a little more useful and specific...in this sample "ActiveMQPropertyConversionException"
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574053059
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AMQPMessageSupport.java
##########
@@ -303,34 +294,11 @@ public static String toAnnotationName(String key) {
return key;
}
-
- public static String toAddress(Destination destination) {
- try {
- if (destination instanceof ActiveMQDestination) {
- return ((ActiveMQDestination) destination).getAddress();
- } else {
- if (destination instanceof Queue) {
- return ((Queue) destination).getQueueName();
- } else if (destination instanceof Topic) {
-
- return ((Topic) destination).getTopicName();
- }
- }
- } catch (JMSException e) {
- // ActiveMQDestination (and most JMS implementations I know) will never throw an Exception here
- // this is here for compilation support (as JMS declares it), and I don't want to propagate exceptions into
- // the converter...
- // and for the possibility of who knows in the future!!!
- logger.warn(e.getMessage(), e);
- }
- return null;
- }
-
public static ServerJMSBytesMessage createBytesMessage(long id, CoreMessageObjectPools coreMessageObjectPools) {
return new ServerJMSBytesMessage(newMessage(id, BYTES_TYPE, coreMessageObjectPools));
}
- public static ServerJMSBytesMessage createBytesMessage(long id, byte[] array, int arrayOffset, int length, CoreMessageObjectPools coreMessageObjectPools) throws JMSException {
+ public static ServerJMSBytesMessage createBytesMessage(long id, byte[] array, int arrayOffset, int length, CoreMessageObjectPools coreMessageObjectPools) throws Exception {
Review comment:
what's the different on throwing something like ArtemisException here?
This is used just for the converters, all that matters is that we capture eventual exceptions and log them.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574049974
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
Review comment:
There is no more JMS on the conversion module..
This is the AMQP Converter.. not the JMS implementation
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574049035
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AmqpCoreConverter.java
##########
@@ -444,7 +446,7 @@ private static void encodeUnsupportedMessagePropertyType(ServerJMSMessage jms, S
}
}
- private static void setProperty(javax.jms.Message msg, String key, Object value) throws JMSException {
+ private static void setProperty(ServerJMSMessage msg, String key, Object value) throws Exception {
Review comment:
This is only used on conversions.. there's no need for anything specifically here... This is just throwing whatever happens underneath.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574004669
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AMQPMessageSupport.java
##########
@@ -54,15 +42,18 @@
import org.apache.qpid.proton.amqp.Symbol;
import org.apache.qpid.proton.amqp.messaging.Data;
import org.apache.qpid.proton.message.Message;
-import org.jboss.logging.Logger;
/**
* Support class containing constant values and static methods that are used to map to / from
* AMQP Message types being sent or received.
*/
public final class AMQPMessageSupport {
- private static final Logger logger = Logger.getLogger(AMQPMessageSupport.class);
+ // These are duplicates from ActiveMQDestination on the jms module.
Review comment:
-1 please dont duplicate stuff. If needed place into some common place
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574050699
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -425,7 +419,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
break;
}
} else if (message instanceof ServerJMSTextMessage) {
- String text = (((TextMessage) message).getText());
+ String text = (((ServerJMSTextMessage) message).getText());
Review comment:
No.. we can't... we need to remove the JMS API implementation from the converter.
Right now the converter is depending on the JMS Client API.
Wildfly for instance is not JMS any longer. It is just simpler to remove the dependency.
This is again just for conversion.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574070715
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
Review comment:
Im really not. Im suggesting drop trying to be JMS at all if you want to remove JMS api stuff here.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574046527
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AMQPMessageSupport.java
##########
@@ -396,7 +364,8 @@ private static CoreMessage newMessage(long id, byte messageType, CoreMessageObje
}
- public static byte destinationType(Destination destination) {
+ // IMPORTANT-TODO: HOW TO GET THIS?
Review comment:
I thought we had fixed this.. I will take a look on this on.. thanks
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#issuecomment-777108190
I reverted the commit. sending a new one tomorrow
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574001644
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AmqpCoreConverter.java
##########
@@ -444,7 +446,7 @@ private static void encodeUnsupportedMessagePropertyType(ServerJMSMessage jms, S
}
}
- private static void setProperty(javax.jms.Message msg, String key, Object value) throws JMSException {
+ private static void setProperty(ServerJMSMessage msg, String key, Object value) throws Exception {
Review comment:
-1 throwing raw exception
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574058863
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -456,7 +450,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
while (true) {
list.add(m.readObject());
}
- } catch (MessageEOFException e) {
+ } catch (Exception e) {
Review comment:
There's nothing else that could happen here...
I don't see the need to duplicate the entire JMS Exception just for these...
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574052387
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -171,15 +165,15 @@ public static AMQPMessage fromCore(ICoreMessage coreMessage, StorageManager stor
properties.setMessageId("ID:" + message.getInnerMessage().getUserID().toString());
}
}
- Destination destination = message.getJMSDestination();
+ SimpleString destination = message.getJMSDestination();
if (destination != null) {
- properties.setTo(toAddress(destination));
- maMap.put(JMS_DEST_TYPE_MSG_ANNOTATION, AMQPMessageSupport.destinationType(destination));
+ properties.setTo(destination.toString());
Review comment:
I dont follow
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574077692
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AmqpCoreConverter.java
##########
@@ -264,8 +266,8 @@ protected static ServerJMSMessage processHeader(ServerJMSMessage jms, Header hea
jms.setLongProperty("JMSXDeliveryCount", header.getDeliveryCount().longValue() + 1);
}
} else {
- jms.setJMSPriority((byte) javax.jms.Message.DEFAULT_PRIORITY);
- jms.setJMSDeliveryMode(DeliveryMode.NON_PERSISTENT);
+ jms.setJMSPriority((byte) MESSAGE_DEFAULT_PRIORITY);
Review comment:
So why not simply use the jakata interfaces in server going forwards
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574068138
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -425,7 +419,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
break;
}
} else if (message instanceof ServerJMSTextMessage) {
- String text = (((TextMessage) message).getText());
+ String text = (((ServerJMSTextMessage) message).getText());
Review comment:
So either then move away from it being JMS this and that. E.g. to convert from/to Core which core is not JMS. Lets just drop JMS properly
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574004052
##########
File path: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java
##########
@@ -42,6 +43,8 @@
private static final long serialVersionUID = 5027962425462382883L;
+ // INFO: These variables are duplicated as part of AMQPMessageSupport in artemis-amqp-protocols
+ // The duplication there is to avoid a dependency on this module from a server's module
Review comment:
Then move them to somewhere unified. Lets not duplicate stuff
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574064380
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMapMessage.java
##########
@@ -137,84 +123,75 @@ public void setObject(final String name, final Object value) throws JMSException
}
TypedProperties.setObjectProperty(new SimpleString(name), val, map);
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public boolean getBoolean(final String name) throws JMSException {
+ public boolean getBoolean(final String name) throws Exception {
try {
return map.getBooleanProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public byte getByte(final String name) throws JMSException {
+ public byte getByte(final String name) throws Exception {
try {
return map.getByteProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
Review comment:
Should not happen doesnt mean it cannot...
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574052644
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
Review comment:
Ok then rename methods, and dont call it JMSxxxxxx
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574072796
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
+ message.setAddress(destination);
}
- @Override
- public final int getJMSDeliveryMode() throws JMSException {
- return message.isDurable() ? DeliveryMode.PERSISTENT : DeliveryMode.NON_PERSISTENT;
+ public final int getJMSDeliveryMode() throws Exception {
+ return message.isDurable() ? DeliveryMode_PERSISTENT : DeliveryMode_NON_PERSISTENT;
}
- @Override
- public final void setJMSDeliveryMode(int deliveryMode) throws JMSException {
- if (deliveryMode == DeliveryMode.PERSISTENT) {
- message.setDurable(true);
- } else if (deliveryMode == DeliveryMode.NON_PERSISTENT) {
- message.setDurable(false);
- } else {
- throw new JMSException("Invalid mode " + deliveryMode);
+ public final void setJMSDeliveryMode(int deliveryMode) throws Exception {
+ switch (deliveryMode) {
+ case DeliveryMode_PERSISTENT:
+ message.setDurable(true);
+ break;
+ case DeliveryMode_NON_PERSISTENT:
+ message.setDurable(false);
+ break;
+ default:
+ throw new Exception("Invalid mode " + deliveryMode);
Review comment:
Thats worse. Surely we should have a specific exception
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574076088
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -425,7 +419,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
break;
}
} else if (message instanceof ServerJMSTextMessage) {
- String text = (((TextMessage) message).getText());
+ String text = (((ServerJMSTextMessage) message).getText());
Review comment:
the AMQP Protocol Head should not depend on JMS at all.. no need for it just for the interfaces definition that are not required.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574013249
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMapMessage.java
##########
@@ -137,84 +123,75 @@ public void setObject(final String name, final Object value) throws JMSException
}
TypedProperties.setObjectProperty(new SimpleString(name), val, map);
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public boolean getBoolean(final String name) throws JMSException {
+ public boolean getBoolean(final String name) throws Exception {
try {
return map.getBooleanProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public byte getByte(final String name) throws JMSException {
+ public byte getByte(final String name) throws Exception {
try {
return map.getByteProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
Review comment:
-1 just throwing a runtime exception. This loses all context about the exception type. Ditto this on all same in this pr
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574077892
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AmqpCoreConverter.java
##########
@@ -264,8 +266,8 @@ protected static ServerJMSMessage processHeader(ServerJMSMessage jms, Header hea
jms.setLongProperty("JMSXDeliveryCount", header.getDeliveryCount().longValue() + 1);
}
} else {
- jms.setJMSPriority((byte) javax.jms.Message.DEFAULT_PRIORITY);
- jms.setJMSDeliveryMode(DeliveryMode.NON_PERSISTENT);
+ jms.setJMSPriority((byte) MESSAGE_DEFAULT_PRIORITY);
Review comment:
Its 1..1 afaik atm
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] asfgit closed pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574078808
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AMQPMessageSupport.java
##########
@@ -303,34 +294,11 @@ public static String toAnnotationName(String key) {
return key;
}
-
- public static String toAddress(Destination destination) {
- try {
- if (destination instanceof ActiveMQDestination) {
- return ((ActiveMQDestination) destination).getAddress();
- } else {
- if (destination instanceof Queue) {
- return ((Queue) destination).getQueueName();
- } else if (destination instanceof Topic) {
-
- return ((Topic) destination).getTopicName();
- }
- }
- } catch (JMSException e) {
- // ActiveMQDestination (and most JMS implementations I know) will never throw an Exception here
- // this is here for compilation support (as JMS declares it), and I don't want to propagate exceptions into
- // the converter...
- // and for the possibility of who knows in the future!!!
- logger.warn(e.getMessage(), e);
- }
- return null;
- }
-
public static ServerJMSBytesMessage createBytesMessage(long id, CoreMessageObjectPools coreMessageObjectPools) {
return new ServerJMSBytesMessage(newMessage(id, BYTES_TYPE, coreMessageObjectPools));
}
- public static ServerJMSBytesMessage createBytesMessage(long id, byte[] array, int arrayOffset, int length, CoreMessageObjectPools coreMessageObjectPools) throws JMSException {
+ public static ServerJMSBytesMessage createBytesMessage(long id, byte[] array, int arrayOffset, int length, CoreMessageObjectPools coreMessageObjectPools) throws Exception {
Review comment:
Differentiate between an exception thats unexpected entirely due to a bug like concurrent modification or NPE from expected ones
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574006006
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -171,15 +165,15 @@ public static AMQPMessage fromCore(ICoreMessage coreMessage, StorageManager stor
properties.setMessageId("ID:" + message.getInnerMessage().getUserID().toString());
}
}
- Destination destination = message.getJMSDestination();
+ SimpleString destination = message.getJMSDestination();
Review comment:
I expect get JMSDestination to return a JMSDestination object not a string
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574074294
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -425,7 +419,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
break;
}
} else if (message instanceof ServerJMSTextMessage) {
- String text = (((TextMessage) message).getText());
+ String text = (((ServerJMSTextMessage) message).getText());
Review comment:
Or even why not just move over to being the Jakarta EE interfaces that replace the older oracle / geronimo ones. As i understand its 1..1 atm
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574074898
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
Review comment:
Or if ita about the move to Jarkata. Why not use the Jarkata interfaces
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574062622
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -171,15 +165,15 @@ public static AMQPMessage fromCore(ICoreMessage coreMessage, StorageManager stor
properties.setMessageId("ID:" + message.getInnerMessage().getUserID().toString());
}
}
- Destination destination = message.getJMSDestination();
+ SimpleString destination = message.getJMSDestination();
if (destination != null) {
- properties.setTo(toAddress(destination));
- maMap.put(JMS_DEST_TYPE_MSG_ANNOTATION, AMQPMessageSupport.destinationType(destination));
+ properties.setTo(destination.toString());
Review comment:
Can we revert master? And then review this properly?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574064100
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
+ message.setAddress(destination);
}
- @Override
- public final int getJMSDeliveryMode() throws JMSException {
- return message.isDurable() ? DeliveryMode.PERSISTENT : DeliveryMode.NON_PERSISTENT;
+ public final int getJMSDeliveryMode() throws Exception {
+ return message.isDurable() ? DeliveryMode_PERSISTENT : DeliveryMode_NON_PERSISTENT;
}
- @Override
- public final void setJMSDeliveryMode(int deliveryMode) throws JMSException {
- if (deliveryMode == DeliveryMode.PERSISTENT) {
- message.setDurable(true);
- } else if (deliveryMode == DeliveryMode.NON_PERSISTENT) {
- message.setDurable(false);
- } else {
- throw new JMSException("Invalid mode " + deliveryMode);
+ public final void setJMSDeliveryMode(int deliveryMode) throws Exception {
+ switch (deliveryMode) {
+ case DeliveryMode_PERSISTENT:
+ message.setDurable(true);
+ break;
+ case DeliveryMode_NON_PERSISTENT:
+ message.setDurable(false);
+ break;
+ default:
+ throw new Exception("Invalid mode " + deliveryMode);
Review comment:
We really should have some exception hierarchy
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574079920
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -425,7 +419,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
break;
}
} else if (message instanceof ServerJMSTextMessage) {
- String text = (((TextMessage) message).getText());
+ String text = (((ServerJMSTextMessage) message).getText());
Review comment:
Then lets drop the JMS constructs here then not this halfway house
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574050181
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -171,15 +165,15 @@ public static AMQPMessage fromCore(ICoreMessage coreMessage, StorageManager stor
properties.setMessageId("ID:" + message.getInnerMessage().getUserID().toString());
}
}
- Destination destination = message.getJMSDestination();
+ SimpleString destination = message.getJMSDestination();
if (destination != null) {
- properties.setTo(toAddress(destination));
- maMap.put(JMS_DEST_TYPE_MSG_ANNOTATION, AMQPMessageSupport.destinationType(destination));
+ properties.setTo(destination.toString());
Review comment:
Same that I said before...
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574018663
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -171,15 +165,15 @@ public static AMQPMessage fromCore(ICoreMessage coreMessage, StorageManager stor
properties.setMessageId("ID:" + message.getInnerMessage().getUserID().toString());
}
}
- Destination destination = message.getJMSDestination();
+ SimpleString destination = message.getJMSDestination();
if (destination != null) {
- properties.setTo(toAddress(destination));
- maMap.put(JMS_DEST_TYPE_MSG_ANNOTATION, AMQPMessageSupport.destinationType(destination));
+ properties.setTo(destination.toString());
Review comment:
Wheres the original logic that was being done inside toAddress
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574071568
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMapMessage.java
##########
@@ -137,84 +123,75 @@ public void setObject(final String name, final Object value) throws JMSException
}
TypedProperties.setObjectProperty(new SimpleString(name), val, map);
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public boolean getBoolean(final String name) throws JMSException {
+ public boolean getBoolean(final String name) throws Exception {
try {
return map.getBooleanProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public byte getByte(final String name) throws JMSException {
+ public byte getByte(final String name) throws Exception {
try {
return map.getByteProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
Review comment:
You may as well just let the original exception bubble out.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574057843
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
Review comment:
you're nit-picking here.. this is going after the JMS implementation.. but without having the actual implementation. It would be easier to keep it the same way.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574065310
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
+ message.setAddress(destination);
}
- @Override
- public final int getJMSDeliveryMode() throws JMSException {
- return message.isDurable() ? DeliveryMode.PERSISTENT : DeliveryMode.NON_PERSISTENT;
+ public final int getJMSDeliveryMode() throws Exception {
+ return message.isDurable() ? DeliveryMode_PERSISTENT : DeliveryMode_NON_PERSISTENT;
}
- @Override
- public final void setJMSDeliveryMode(int deliveryMode) throws JMSException {
- if (deliveryMode == DeliveryMode.PERSISTENT) {
- message.setDurable(true);
- } else if (deliveryMode == DeliveryMode.NON_PERSISTENT) {
- message.setDurable(false);
- } else {
- throw new JMSException("Invalid mode " + deliveryMode);
+ public final void setJMSDeliveryMode(int deliveryMode) throws Exception {
+ switch (deliveryMode) {
+ case DeliveryMode_PERSISTENT:
+ message.setDurable(true);
+ break;
+ case DeliveryMode_NON_PERSISTENT:
+ message.setDurable(false);
+ break;
+ default:
+ throw new Exception("Invalid mode " + deliveryMode);
Review comment:
Why do you suggest here? I don't want to copy, paste and duplicate all the Message Interfaces and all the Exceptions as part of the converter. That would be a waste.
Message APIs and Exception types is pretty much what I use from the JMS API. If you want a concrete exception, you are pretty much opposing to remove the dependency? I don't see a need here really.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574068138
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -425,7 +419,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
break;
}
} else if (message instanceof ServerJMSTextMessage) {
- String text = (((TextMessage) message).getText());
+ String text = (((ServerJMSTextMessage) message).getText());
Review comment:
So either this is JMS and keep the api or if were dropping that in conversion then move away from it being JMS this and that. E.g. to convert from/to Core which core is not JMS. Lets just drop JMS properly
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574019618
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -456,7 +450,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
while (true) {
list.add(m.readObject());
}
- } catch (MessageEOFException e) {
+ } catch (Exception e) {
Review comment:
This should only be catching/ doing this for EOF exceptions not for all!
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574053100
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -171,15 +165,15 @@ public static AMQPMessage fromCore(ICoreMessage coreMessage, StorageManager stor
properties.setMessageId("ID:" + message.getInnerMessage().getUserID().toString());
}
}
- Destination destination = message.getJMSDestination();
+ SimpleString destination = message.getJMSDestination();
Review comment:
But then rename the method. Leaving it named JMSxxxxx means you expect a JMS thing
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574068638
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMapMessage.java
##########
@@ -137,84 +123,75 @@ public void setObject(final String name, final Object value) throws JMSException
}
TypedProperties.setObjectProperty(new SimpleString(name), val, map);
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public boolean getBoolean(final String name) throws JMSException {
+ public boolean getBoolean(final String name) throws Exception {
try {
return map.getBooleanProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public byte getByte(final String name) throws JMSException {
+ public byte getByte(final String name) throws Exception {
try {
return map.getByteProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
Review comment:
It can... a RuntimeException that's captured on the converter is all we need.
What good would make if I created a generic ConversionException and replaced all the JMSException by ConversionException here?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574061351
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -171,15 +165,15 @@ public static AMQPMessage fromCore(ICoreMessage coreMessage, StorageManager stor
properties.setMessageId("ID:" + message.getInnerMessage().getUserID().toString());
}
}
- Destination destination = message.getJMSDestination();
+ SimpleString destination = message.getJMSDestination();
if (destination != null) {
- properties.setTo(toAddress(destination));
- maMap.put(JMS_DEST_TYPE_MSG_ANNOTATION, AMQPMessageSupport.destinationType(destination));
+ properties.setTo(destination.toString());
Review comment:
I see, there's a bug on this part, I'm fixing it. there's actually a test failure caused by 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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574057514
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
+ message.setAddress(destination);
}
- @Override
- public final int getJMSDeliveryMode() throws JMSException {
- return message.isDurable() ? DeliveryMode.PERSISTENT : DeliveryMode.NON_PERSISTENT;
+ public final int getJMSDeliveryMode() throws Exception {
+ return message.isDurable() ? DeliveryMode_PERSISTENT : DeliveryMode_NON_PERSISTENT;
}
- @Override
- public final void setJMSDeliveryMode(int deliveryMode) throws JMSException {
- if (deliveryMode == DeliveryMode.PERSISTENT) {
- message.setDurable(true);
- } else if (deliveryMode == DeliveryMode.NON_PERSISTENT) {
- message.setDurable(false);
- } else {
- throw new JMSException("Invalid mode " + deliveryMode);
+ public final void setJMSDeliveryMode(int deliveryMode) throws Exception {
+ switch (deliveryMode) {
+ case DeliveryMode_PERSISTENT:
+ message.setDurable(true);
+ break;
+ case DeliveryMode_NON_PERSISTENT:
+ message.setDurable(false);
+ break;
+ default:
+ throw new Exception("Invalid mode " + deliveryMode);
Review comment:
ok, it will be a RuntimeException.. it can't be anything else on the conversion.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574017456
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
+ message.setAddress(destination);
}
- @Override
- public final int getJMSDeliveryMode() throws JMSException {
- return message.isDurable() ? DeliveryMode.PERSISTENT : DeliveryMode.NON_PERSISTENT;
+ public final int getJMSDeliveryMode() throws Exception {
+ return message.isDurable() ? DeliveryMode_PERSISTENT : DeliveryMode_NON_PERSISTENT;
}
- @Override
- public final void setJMSDeliveryMode(int deliveryMode) throws JMSException {
- if (deliveryMode == DeliveryMode.PERSISTENT) {
- message.setDurable(true);
- } else if (deliveryMode == DeliveryMode.NON_PERSISTENT) {
- message.setDurable(false);
- } else {
- throw new JMSException("Invalid mode " + deliveryMode);
+ public final void setJMSDeliveryMode(int deliveryMode) throws Exception {
+ switch (deliveryMode) {
+ case DeliveryMode_PERSISTENT:
Review comment:
Can we make a class with the constants inside so its a bit like before. This just is getting messy now
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] tabish121 commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
tabish121 commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r573953735
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AMQPMessageSupport.java
##########
@@ -396,7 +364,8 @@ private static CoreMessage newMessage(long id, byte messageType, CoreMessageObje
}
- public static byte destinationType(Destination destination) {
+ // IMPORTANT-TODO: HOW TO GET THIS?
Review comment:
I'm confused, if this is important than why commit the change before having figured that bit out, if it isn't then why leave this whole bit of commented out code here ?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r573999065
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AMQPMessageSupport.java
##########
@@ -303,34 +294,11 @@ public static String toAnnotationName(String key) {
return key;
}
-
- public static String toAddress(Destination destination) {
Review comment:
Wheres this logic gone to?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] ehsavoie commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
ehsavoie commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574341154
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AmqpCoreConverter.java
##########
@@ -264,8 +266,8 @@ protected static ServerJMSMessage processHeader(ServerJMSMessage jms, Header hea
jms.setLongProperty("JMSXDeliveryCount", header.getDeliveryCount().longValue() + 1);
}
} else {
- jms.setJMSPriority((byte) javax.jms.Message.DEFAULT_PRIORITY);
- jms.setJMSDeliveryMode(DeliveryMode.NON_PERSISTENT);
+ jms.setJMSPriority((byte) MESSAGE_DEFAULT_PRIORITY);
Review comment:
I'm not sure that we should use JMS or Jakarta Messaging API to convert between core and amqp messsages
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574075603
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
Review comment:
why the converter needs JMS at all? it just needs to know to convert it.. nothing else.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574078808
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AMQPMessageSupport.java
##########
@@ -303,34 +294,11 @@ public static String toAnnotationName(String key) {
return key;
}
-
- public static String toAddress(Destination destination) {
- try {
- if (destination instanceof ActiveMQDestination) {
- return ((ActiveMQDestination) destination).getAddress();
- } else {
- if (destination instanceof Queue) {
- return ((Queue) destination).getQueueName();
- } else if (destination instanceof Topic) {
-
- return ((Topic) destination).getTopicName();
- }
- }
- } catch (JMSException e) {
- // ActiveMQDestination (and most JMS implementations I know) will never throw an Exception here
- // this is here for compilation support (as JMS declares it), and I don't want to propagate exceptions into
- // the converter...
- // and for the possibility of who knows in the future!!!
- logger.warn(e.getMessage(), e);
- }
- return null;
- }
-
public static ServerJMSBytesMessage createBytesMessage(long id, CoreMessageObjectPools coreMessageObjectPools) {
return new ServerJMSBytesMessage(newMessage(id, BYTES_TYPE, coreMessageObjectPools));
}
- public static ServerJMSBytesMessage createBytesMessage(long id, byte[] array, int arrayOffset, int length, CoreMessageObjectPools coreMessageObjectPools) throws JMSException {
+ public static ServerJMSBytesMessage createBytesMessage(long id, byte[] array, int arrayOffset, int length, CoreMessageObjectPools coreMessageObjectPools) throws Exception {
Review comment:
Differentiate between an exception thats unexpected entirely due to a bug like concurrent modification or NPE from semi expected ones
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574063119
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
+ message.setAddress(destination);
}
- @Override
- public final int getJMSDeliveryMode() throws JMSException {
- return message.isDurable() ? DeliveryMode.PERSISTENT : DeliveryMode.NON_PERSISTENT;
+ public final int getJMSDeliveryMode() throws Exception {
+ return message.isDurable() ? DeliveryMode_PERSISTENT : DeliveryMode_NON_PERSISTENT;
}
- @Override
- public final void setJMSDeliveryMode(int deliveryMode) throws JMSException {
- if (deliveryMode == DeliveryMode.PERSISTENT) {
- message.setDurable(true);
- } else if (deliveryMode == DeliveryMode.NON_PERSISTENT) {
- message.setDurable(false);
- } else {
- throw new JMSException("Invalid mode " + deliveryMode);
+ public final void setJMSDeliveryMode(int deliveryMode) throws Exception {
+ switch (deliveryMode) {
+ case DeliveryMode_PERSISTENT:
+ message.setDurable(true);
+ break;
+ case DeliveryMode_NON_PERSISTENT:
+ message.setDurable(false);
+ break;
+ default:
+ throw new Exception("Invalid mode " + deliveryMode);
Review comment:
But thats even worse
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574052199
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -425,7 +419,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
break;
}
} else if (message instanceof ServerJMSTextMessage) {
- String text = (((TextMessage) message).getText());
+ String text = (((ServerJMSTextMessage) message).getText());
Review comment:
I got that you want to remove the current api. But still can have an interface even if an internal one
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574019026
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -425,7 +419,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
break;
}
} else if (message instanceof ServerJMSTextMessage) {
- String text = (((TextMessage) message).getText());
+ String text = (((ServerJMSTextMessage) message).getText());
Review comment:
Can we use an interface here. And not concrete classes
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574000970
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AmqpCoreConverter.java
##########
@@ -264,8 +266,8 @@ protected static ServerJMSMessage processHeader(ServerJMSMessage jms, Header hea
jms.setLongProperty("JMSXDeliveryCount", header.getDeliveryCount().longValue() + 1);
}
} else {
- jms.setJMSPriority((byte) javax.jms.Message.DEFAULT_PRIORITY);
- jms.setJMSDeliveryMode(DeliveryMode.NON_PERSISTENT);
+ jms.setJMSPriority((byte) MESSAGE_DEFAULT_PRIORITY);
Review comment:
I dont understand the driver here. This is handling JMS specific stuff, so it just makes sense to use the api values and not be making our own
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574049584
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -171,15 +165,15 @@ public static AMQPMessage fromCore(ICoreMessage coreMessage, StorageManager stor
properties.setMessageId("ID:" + message.getInnerMessage().getUserID().toString());
}
}
- Destination destination = message.getJMSDestination();
+ SimpleString destination = message.getJMSDestination();
Review comment:
This is for Conversion only.. there's no JMSDestination.. we would need to depend on artemis-jms-impl that depends on JMS.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574016641
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
Review comment:
-1 the name of this suggests this should be a jms object.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574079920
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -425,7 +419,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
break;
}
} else if (message instanceof ServerJMSTextMessage) {
- String text = (((TextMessage) message).getText());
+ String text = (((ServerJMSTextMessage) message).getText());
Review comment:
Then lets drop the JMS constructs fully here then not this halfway house
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574076932
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AmqpCoreConverter.java
##########
@@ -444,7 +446,7 @@ private static void encodeUnsupportedMessagePropertyType(ServerJMSMessage jms, S
}
}
- private static void setProperty(javax.jms.Message msg, String key, Object value) throws JMSException {
+ private static void setProperty(ServerJMSMessage msg, String key, Object value) throws Exception {
Review comment:
Then maybe have a single base exception for these. At least that way can still differentiate between those and other possible unexpected exceptions that could arise from a 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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574061351
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -171,15 +165,15 @@ public static AMQPMessage fromCore(ICoreMessage coreMessage, StorageManager stor
properties.setMessageId("ID:" + message.getInnerMessage().getUserID().toString());
}
}
- Destination destination = message.getJMSDestination();
+ SimpleString destination = message.getJMSDestination();
if (destination != null) {
- properties.setTo(toAddress(destination));
- maMap.put(JMS_DEST_TYPE_MSG_ANNOTATION, AMQPMessageSupport.destinationType(destination));
+ properties.setTo(destination.toString());
Review comment:
there's a bug on this part, I'm fixing 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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574061071
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMapMessage.java
##########
@@ -137,84 +123,75 @@ public void setObject(final String name, final Object value) throws JMSException
}
TypedProperties.setObjectProperty(new SimpleString(name), val, map);
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public boolean getBoolean(final String name) throws JMSException {
+ public boolean getBoolean(final String name) throws Exception {
try {
return map.getBooleanProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
}
}
- @Override
- public byte getByte(final String name) throws JMSException {
+ public byte getByte(final String name) throws Exception {
try {
return map.getByteProperty(new SimpleString(name));
} catch (ActiveMQPropertyConversionException e) {
- throw new MessageFormatException(e.getMessage());
+ throw new RuntimeException(e.getMessage());
Review comment:
This should not really happen at the Converters.
Creating our own Exception here wouldn't make it more useful.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574062357
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -425,7 +419,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
break;
}
} else if (message instanceof ServerJMSTextMessage) {
- String text = (((TextMessage) message).getText());
+ String text = (((ServerJMSTextMessage) message).getText());
Review comment:
I would then duplicate the whole JMS API internally for the conversion.
Concrete types is all we need here. The only reason I added these classes in the first place was to do the conversion.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574053819
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AmqpCoreConverter.java
##########
@@ -264,8 +266,8 @@ protected static ServerJMSMessage processHeader(ServerJMSMessage jms, Header hea
jms.setLongProperty("JMSXDeliveryCount", header.getDeliveryCount().longValue() + 1);
}
} else {
- jms.setJMSPriority((byte) javax.jms.Message.DEFAULT_PRIORITY);
- jms.setJMSDeliveryMode(DeliveryMode.NON_PERSISTENT);
+ jms.setJMSPriority((byte) MESSAGE_DEFAULT_PRIORITY);
Review comment:
The AMQP Protocol implementation is depending on the JMS implementation. Going forward we have Jakarta and JMS (at the point they are a transformation). There makes no sense to force a direct dependency on it.
Wildfly is having to import JMS just because of 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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574089979
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
+ message.setAddress(destination);
}
- @Override
- public final int getJMSDeliveryMode() throws JMSException {
- return message.isDurable() ? DeliveryMode.PERSISTENT : DeliveryMode.NON_PERSISTENT;
+ public final int getJMSDeliveryMode() throws Exception {
+ return message.isDurable() ? DeliveryMode_PERSISTENT : DeliveryMode_NON_PERSISTENT;
}
- @Override
- public final void setJMSDeliveryMode(int deliveryMode) throws JMSException {
- if (deliveryMode == DeliveryMode.PERSISTENT) {
- message.setDurable(true);
- } else if (deliveryMode == DeliveryMode.NON_PERSISTENT) {
- message.setDurable(false);
- } else {
- throw new JMSException("Invalid mode " + deliveryMode);
+ public final void setJMSDeliveryMode(int deliveryMode) throws Exception {
+ switch (deliveryMode) {
+ case DeliveryMode_PERSISTENT:
Review comment:
will do that
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574015555
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/jms/ServerJMSMessage.java
##########
@@ -137,226 +131,178 @@ public final String getJMSCorrelationID() throws JMSException {
}
}
- @Override
- public final void setJMSCorrelationID(String correlationID) throws JMSException {
+ public final void setJMSCorrelationID(String correlationID) throws Exception {
message.setCorrelationID(correlationID);
}
- @Override
- public final Destination getJMSReplyTo() throws JMSException {
- SimpleString reply = MessageUtil.getJMSReplyTo(message);
- if (reply != null) {
- return ActiveMQDestination.fromPrefixedName(reply.toString());
- } else {
- return null;
- }
+ public final SimpleString getJMSReplyTo() throws Exception {
+ return MessageUtil.getJMSReplyTo(message);
}
- @Override
- public final void setJMSReplyTo(Destination replyTo) throws JMSException {
- MessageUtil.setJMSReplyTo(message, replyTo == null ? null : ((ActiveMQDestination) replyTo).getSimpleAddress());
+ public final void setJMSReplyTo(String replyTo) throws Exception {
+ MessageUtil.setJMSReplyTo(message, SimpleString.toSimpleString(replyTo));
}
- @Override
- public Destination getJMSDestination() throws JMSException {
- return ActiveMQDestination.createDestination(message.getRoutingType(), message.getAddressSimpleString());
+ public SimpleString getJMSDestination() throws Exception {
+ return message.getAddressSimpleString();
}
- @Override
- public final void setJMSDestination(Destination destination) throws JMSException {
- if (destination == null) {
- message.setAddress((SimpleString) null);
- } else {
- message.setAddress(((ActiveMQDestination) destination).getSimpleAddress());
- }
-
+ public final void setJMSDestination(String destination) throws Exception {
+ message.setAddress(destination);
}
- @Override
- public final int getJMSDeliveryMode() throws JMSException {
- return message.isDurable() ? DeliveryMode.PERSISTENT : DeliveryMode.NON_PERSISTENT;
+ public final int getJMSDeliveryMode() throws Exception {
+ return message.isDurable() ? DeliveryMode_PERSISTENT : DeliveryMode_NON_PERSISTENT;
}
- @Override
- public final void setJMSDeliveryMode(int deliveryMode) throws JMSException {
- if (deliveryMode == DeliveryMode.PERSISTENT) {
- message.setDurable(true);
- } else if (deliveryMode == DeliveryMode.NON_PERSISTENT) {
- message.setDurable(false);
- } else {
- throw new JMSException("Invalid mode " + deliveryMode);
+ public final void setJMSDeliveryMode(int deliveryMode) throws Exception {
+ switch (deliveryMode) {
+ case DeliveryMode_PERSISTENT:
+ message.setDurable(true);
+ break;
+ case DeliveryMode_NON_PERSISTENT:
+ message.setDurable(false);
+ break;
+ default:
+ throw new Exception("Invalid mode " + deliveryMode);
Review comment:
-1 throwing raw exception.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#issuecomment-776944761
Im tempted to just revert this on master tbh. This is a massive change, that cannot be reviewed quickly. Ive just spent 15 mins and still got loads of classes to go and wont get through till tomorrow.
Can you revert this on master @clebertsuconic and let everyone have time to discuss/review
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r573999541
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AMQPMessageSupport.java
##########
@@ -303,34 +294,11 @@ public static String toAnnotationName(String key) {
return key;
}
-
- public static String toAddress(Destination destination) {
- try {
- if (destination instanceof ActiveMQDestination) {
- return ((ActiveMQDestination) destination).getAddress();
- } else {
- if (destination instanceof Queue) {
- return ((Queue) destination).getQueueName();
- } else if (destination instanceof Topic) {
-
- return ((Topic) destination).getTopicName();
- }
- }
- } catch (JMSException e) {
- // ActiveMQDestination (and most JMS implementations I know) will never throw an Exception here
- // this is here for compilation support (as JMS declares it), and I don't want to propagate exceptions into
- // the converter...
- // and for the possibility of who knows in the future!!!
- logger.warn(e.getMessage(), e);
- }
- return null;
- }
-
public static ServerJMSBytesMessage createBytesMessage(long id, CoreMessageObjectPools coreMessageObjectPools) {
return new ServerJMSBytesMessage(newMessage(id, BYTES_TYPE, coreMessageObjectPools));
}
- public static ServerJMSBytesMessage createBytesMessage(long id, byte[] array, int arrayOffset, int length, CoreMessageObjectPools coreMessageObjectPools) throws JMSException {
+ public static ServerJMSBytesMessage createBytesMessage(long id, byte[] array, int arrayOffset, int length, CoreMessageObjectPools coreMessageObjectPools) throws Exception {
Review comment:
Can we avoid throwing raw exception
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#issuecomment-776938244
@clebertsuconic can you please let PRs be open for a day or so to give people time to review. This was opened 2hrs ago and merged under an hour of being opened
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574062357
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -425,7 +419,7 @@ private static Section convertBody(ServerJMSMessage message, Map<Symbol, Object>
break;
}
} else if (message instanceof ServerJMSTextMessage) {
- String text = (((TextMessage) message).getText());
+ String text = (((ServerJMSTextMessage) message).getText());
Review comment:
I would then duplicate the whole JMS API internally for the conversion.
That is a BytesMessage, a TextMessage ...etc... etc... When all we need is the Concrete Type. This is not supposed to used by any users other than the conversion.
Notice this ServerJMSTextMessage is a class that only lives as part of the AMQP implementation.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#issuecomment-777165629
followed up with #3450
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3448: [ARTEMIS-3113]: Artemis AMQP shouldn't depend on JMS.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3448:
URL: https://github.com/apache/activemq-artemis/pull/3448#discussion_r574066419
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/CoreAmqpConverter.java
##########
@@ -171,15 +165,15 @@ public static AMQPMessage fromCore(ICoreMessage coreMessage, StorageManager stor
properties.setMessageId("ID:" + message.getInnerMessage().getUserID().toString());
}
}
- Destination destination = message.getJMSDestination();
+ SimpleString destination = message.getJMSDestination();
if (destination != null) {
- properties.setTo(toAddress(destination));
- maMap.put(JMS_DEST_TYPE_MSG_ANNOTATION, AMQPMessageSupport.destinationType(destination));
+ properties.setTo(destination.toString());
Review comment:
@michaelandrepearce I did not want to depend on JMS at the first time I implemented the converter. I did it back then because it was the easy route. (copy and paste the client API, depend on the interfaces and get done with it).
Now I need to remove the dependency. Using concrete classes is the way to go here.. I don't think there's much to discuss. .unless everybody is opposing to this change.. which I can't see it as a bad thing.
----------------------------------------------------------------
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