You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by jbertram <gi...@git.apache.org> on 2018/01/16 15:16:06 UTC
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
GitHub user jbertram opened a pull request:
https://github.com/apache/activemq-artemis/pull/1778
ARTEMIS-1609 restore 'name' to ActiveMQDestination
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-1609
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1778.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1778
----
commit 264eec2bf09db422c328edcf5150d16ad0e8ac9a
Author: Justin Bertram <jb...@...>
Date: 2018-01-16T15:15:00Z
ARTEMIS-1609 restore 'name' to ActiveMQDestination
----
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r162116039
--- Diff: tests/compatibility-tests/src/test/java/org/apache/activemq/artemis/tests/compatibility/SerializationTest.java ---
@@ -91,15 +91,15 @@ public void afterTest() {
@Test
public void testSerializeFactory() throws Throwable {
File file = serverFolder.newFile("objects.ser");
- callScript(senderClassloader, "serial/serial.groovy", file.getAbsolutePath(), "write");
- callScript(receiverClassloader, "serial/serial.groovy", file.getAbsolutePath(), "read");
+ callScript(senderClassloader, "serial/serial.groovy", file.getAbsolutePath(), "write", sender);
+ callScript(receiverClassloader, "serial/serial.groovy", file.getAbsolutePath(), "read", receiver);
}
@Test
public void testJBMSerializeFactory() throws Throwable {
File file = serverFolder.newFile("objectsjbm.ser");
- callScript(senderClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "write");
- callScript(receiverClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "read");
+ callScript(senderClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "write", sender);
--- End diff --
what's the sender/receiver for?
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r162119392
--- Diff: tests/compatibility-tests/src/test/java/org/apache/activemq/artemis/tests/compatibility/SerializationTest.java ---
@@ -91,15 +91,15 @@ public void afterTest() {
@Test
public void testSerializeFactory() throws Throwable {
File file = serverFolder.newFile("objects.ser");
- callScript(senderClassloader, "serial/serial.groovy", file.getAbsolutePath(), "write");
- callScript(receiverClassloader, "serial/serial.groovy", file.getAbsolutePath(), "read");
+ callScript(senderClassloader, "serial/serial.groovy", file.getAbsolutePath(), "write", sender);
+ callScript(receiverClassloader, "serial/serial.groovy", file.getAbsolutePath(), "read", receiver);
}
@Test
public void testJBMSerializeFactory() throws Throwable {
File file = serverFolder.newFile("objectsjbm.ser");
- callScript(senderClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "write");
- callScript(receiverClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "read");
+ callScript(senderClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "write", sender);
--- End diff --
It's the version used by the script to determine which destination constructor to use.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r162105330
--- Diff: tests/compatibility-tests/src/main/resources/serial/serial.groovy ---
@@ -24,30 +24,40 @@ import org.apache.activemq.artemis.jms.client.*
file = arg[0]
method = arg[1]
-System.out.println("File::" + file);
+version = arg[2]
+System.out.println("File::" + file)
if (method.equals("write")) {
cf = new ActiveMQConnectionFactory("tcp://localhost:61616?confirmationWindowSize=1048576&blockOnDurableSend=false");
queue = new ActiveMQQueue("queue");
topic = new ActiveMQTopic("topic")
+ if (version.equals("ARTEMIS-SNAPSHOT")) {
+ destination = new ActiveMQDestination("address", "name", ActiveMQDestination.TYPE.DESTINATION, null)
--- End diff --
Fair enough, you sold me.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161978381
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -231,6 +239,16 @@ public static ActiveMQTemporaryTopic createTemporaryTopic(String address) {
*/
private SimpleString simpleAddress;
+ /**
+ * Needed for serialization backwards compatibility.
+ */
+ private String address;
--- End diff --
Can we mark as deprecated
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r162105397
--- Diff: tests/compatibility-tests/src/main/resources/serial/jbmserial.groovy ---
@@ -49,6 +50,11 @@ if (method.equals("write")) {
topic = new ActiveMQTopic("topic")
temporary = ActiveMQDestination.createTemporaryQueue("whatever")
temporaryTopic = ActiveMQDestination.createTemporaryTopic("whatever")
+ if (version.equals("ARTEMIS-SNAPSHOT")) {
+ destination = new ActiveMQDestination("address", "name", ActiveMQDestination.TYPE.DESTINATION, null)
--- End diff --
sold.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161978353
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -231,6 +239,16 @@ public static ActiveMQTemporaryTopic createTemporaryTopic(String address) {
*/
private SimpleString simpleAddress;
+ /**
+ * Needed for serialization backwards compatibility.
+ */
+ private String address;
+
+ /**
+ * The "JMS" name of the destination. Needed for serialization backwards compatibility.
+ */
+ private String name;
--- End diff --
Can we mark as deprecated
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by jmesnil <gi...@git.apache.org>.
Github user jmesnil commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161789136
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -309,7 +330,7 @@ public SimpleString getSimpleAddress() {
}
public String getName() {
- return simpleAddress.toString();
+ return name == null ? simpleAddress.toString() : name.toString();
--- End diff --
if we use `address` instead of `null` when creating an ActiveMQDestination, we ensure that the fields are always non-null and this method can just return `name.toString()`
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r162092404
--- Diff: tests/compatibility-tests/src/main/resources/serial/jbmserial.groovy ---
@@ -49,6 +50,11 @@ if (method.equals("write")) {
topic = new ActiveMQTopic("topic")
temporary = ActiveMQDestination.createTemporaryQueue("whatever")
temporaryTopic = ActiveMQDestination.createTemporaryTopic("whatever")
+ if (version.equals("ARTEMIS-SNAPSHOT")) {
+ destination = new ActiveMQDestination("address", "name", ActiveMQDestination.TYPE.DESTINATION, null)
--- End diff --
This is for backwards compatibility. We need to use the field to validate the older format. it's good to be here.
---
[GitHub] activemq-artemis issue #1778: ARTEMIS-1609 restore 'name' to ActiveMQDestina...
Posted by jmesnil <gi...@git.apache.org>.
Github user jmesnil commented on the issue:
https://github.com/apache/activemq-artemis/pull/1778
@jbertram Could you also cherry pick https://github.com/jmesnil/activemq-artemis/commit/7769d43b610cf10dd5f2d82c1d0a3de3385109f0
That's built on top of what you are doing so that the (deprecated) JMSServerManager can specify the name of the JMS destinations.
(I promise I'll move away from using this class soon :)
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161978201
--- Diff: artemis-jms-server/src/main/java/org/apache/activemq/artemis/jms/server/JMSServerManager.java ---
@@ -60,6 +60,24 @@ boolean createQueue(boolean storeConfig,
boolean durable,
String... bindings) throws Exception;
+ /**
+ * Creates a JMS Queue.
+ *
+ * @param queueName The name of the core queue to create
+ * @param jmsQueueName the name of this JMS queue
+ * @param selectorString
+ * @param durable
+ * @return true if the queue is created or if it existed and was added to
+ * the Binding Registry
+ * @throws Exception if problems were encountered creating the queue.
+ */
+ boolean createQueue(boolean storeConfig,
--- End diff --
If this is for back compatibility for the ActiveMQDestination, why are we adding new methods to JMSServerManager, if anything "name" should be deprecated.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161978819
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQTopic.java ---
@@ -40,8 +40,12 @@ public ActiveMQTopic(final String address) {
this(address, false);
}
+ public ActiveMQTopic(final String address, final String name) {
--- End diff --
Please mark as deprecated
---
[GitHub] activemq-artemis issue #1778: ARTEMIS-1609 restore 'name' to ActiveMQDestina...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1778
@clebertsuconic, will do.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161868236
--- Diff: tests/compatibility-tests/src/main/resources/serial/serial.groovy ---
@@ -24,30 +24,46 @@ import org.apache.activemq.artemis.jms.client.*
file = arg[0]
method = arg[1]
-System.out.println("File::" + file);
+version = arg[2]
+System.out.println("File::" + file)
if (method.equals("write")) {
cf = new ActiveMQConnectionFactory("tcp://localhost:61616?confirmationWindowSize=1048576&blockOnDurableSend=false");
queue = new ActiveMQQueue("queue");
topic = new ActiveMQTopic("topic")
+ if (version.equals("ARTEMIS-SNAPSHOT")) {
+ destination = new ActiveMQDestination("address", "name", ActiveMQDestination.TYPE.DESTINATION, null)
+ } else if (version.equals("ARTEMIS-155")) {
+ destination = new ActiveMQDestination("address", "name", false, true, null)
+ }
ObjectOutputStream objectOutputStream = new ObjectOutputStream(new FileOutputStream(file));
objectOutputStream.writeObject(cf);
objectOutputStream.writeObject(queue)
objectOutputStream.writeObject(topic)
+ if (version.startsWith("ARTEMIS")) {
+ objectOutputStream.writeObject(destination)
+ }
objectOutputStream.close();
} else {
ObjectInputStream inputStream = new ObjectInputStream(new FileInputStream(file))
cf = inputStream.readObject();
queue = inputStream.readObject()
topic = inputStream.readObject()
+ if (version.startsWith("ARTEMIS")) {
+ destination = inputStream.readObject()
+ }
inputStream.close();
}
GroovyRun.assertTrue(!cf.getServerLocator().isBlockOnDurableSend());
-GroovyRun.assertEquals(1048576, cf.getServerLocator().getConfirmationWindowSize());
+GroovyRun.assertEquals(1048576, cf.getServerLocator().getConfirmationWindowSize())
+if (version.startsWith("ARTEMIS")) {
--- End diff --
you don't need that if ^^^
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161981000
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -277,6 +305,7 @@ public void setSimpleAddress(SimpleString address) {
if (address == null) {
throw new IllegalArgumentException("address cannot be null");
}
+ this.address = address.toString();
--- End diff --
should also make it set deprecated name field.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161865199
--- Diff: tests/compatibility-tests/src/main/resources/serial/serial.groovy ---
@@ -24,30 +24,46 @@ import org.apache.activemq.artemis.jms.client.*
file = arg[0]
method = arg[1]
-System.out.println("File::" + file);
+version = arg[2]
+System.out.println("File::" + file)
if (method.equals("write")) {
cf = new ActiveMQConnectionFactory("tcp://localhost:61616?confirmationWindowSize=1048576&blockOnDurableSend=false");
queue = new ActiveMQQueue("queue");
topic = new ActiveMQTopic("topic")
+ if (version.equals("ARTEMIS-SNAPSHOT")) {
+ destination = new ActiveMQDestination("address", "name", ActiveMQDestination.TYPE.DESTINATION, null)
+ } else if (version.equals("ARTEMIS-155")) {
+ destination = new ActiveMQDestination("address", "name", false, true, null)
+ }
ObjectOutputStream objectOutputStream = new ObjectOutputStream(new FileOutputStream(file));
objectOutputStream.writeObject(cf);
objectOutputStream.writeObject(queue)
objectOutputStream.writeObject(topic)
+ if (version.startsWith("ARTEMIS")) {
--- End diff --
why check the version here? just keep what it was.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r162082434
--- Diff: artemis-jms-server/src/main/java/org/apache/activemq/artemis/jms/server/JMSServerManager.java ---
@@ -60,6 +60,24 @@ boolean createQueue(boolean storeConfig,
boolean durable,
String... bindings) throws Exception;
+ /**
+ * Creates a JMS Queue.
+ *
+ * @param queueName The name of the core queue to create
+ * @param jmsQueueName the name of this JMS queue
+ * @param selectorString
+ * @param durable
+ * @return true if the queue is created or if it existed and was added to
+ * the Binding Registry
+ * @throws Exception if problems were encountered creating the queue.
+ */
+ boolean createQueue(boolean storeConfig,
--- End diff --
The whole JMSServerManager class is already marked as deprecated. The reason these additions are here is because the JMSServerManager was never updated to deal with the new address model since it was deprecated at the same time the address model was changed. However, integrators which previously used the JMSServerManager may need these new methods while they transition away from using the JMSServerManager to using the ActiveMQServer instead.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1778
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161978616
--- Diff: artemis-jms-server/src/main/java/org/apache/activemq/artemis/jms/server/impl/JMSServerManagerImpl.java ---
@@ -465,11 +467,17 @@ public synchronized boolean createQueue(final boolean storeConfig,
final String selectorString,
final boolean durable,
final String... bindings) throws Exception {
- return internalCreateJMSQueue(storeConfig, queueName, selectorString, durable, false, bindings);
+ return internalCreateJMSQueue(storeConfig, queueName, queueName, selectorString, durable, false, bindings);
+ }
+
+ @Override
+ public boolean createQueue(boolean storeConfig, String queueName, String jmsQueueName, String selectorString, boolean durable, String... bindings) throws Exception {
--- End diff --
As per other comment, not sure why adding new methods here. If the reason for this change is just back compatibility of ActiveMQDestination.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161979093
--- Diff: tests/compatibility-tests/src/main/resources/serial/jbmserial.groovy ---
@@ -49,6 +50,11 @@ if (method.equals("write")) {
topic = new ActiveMQTopic("topic")
temporary = ActiveMQDestination.createTemporaryQueue("whatever")
temporaryTopic = ActiveMQDestination.createTemporaryTopic("whatever")
+ if (version.equals("ARTEMIS-SNAPSHOT")) {
+ destination = new ActiveMQDestination("address", "name", ActiveMQDestination.TYPE.DESTINATION, null)
--- End diff --
Same as other comment, for 2.x this really should be using the non "name" constructor this constructor should be deprecated.
---
[GitHub] activemq-artemis issue #1778: ARTEMIS-1609 restore 'name' to ActiveMQDestina...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1778
@jbertram Can you please change SerializationTest on the compatibility test to make sure such field is translating correctly?
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161868488
--- Diff: tests/compatibility-tests/src/main/resources/serial/serial.groovy ---
@@ -24,30 +24,46 @@ import org.apache.activemq.artemis.jms.client.*
file = arg[0]
method = arg[1]
-System.out.println("File::" + file);
+version = arg[2]
+System.out.println("File::" + file)
if (method.equals("write")) {
cf = new ActiveMQConnectionFactory("tcp://localhost:61616?confirmationWindowSize=1048576&blockOnDurableSend=false");
queue = new ActiveMQQueue("queue");
topic = new ActiveMQTopic("topic")
+ if (version.equals("ARTEMIS-SNAPSHOT")) {
+ destination = new ActiveMQDestination("address", "name", ActiveMQDestination.TYPE.DESTINATION, null)
+ } else if (version.equals("ARTEMIS-155")) {
+ destination = new ActiveMQDestination("address", "name", false, true, null)
+ }
ObjectOutputStream objectOutputStream = new ObjectOutputStream(new FileOutputStream(file));
objectOutputStream.writeObject(cf);
objectOutputStream.writeObject(queue)
objectOutputStream.writeObject(topic)
+ if (version.startsWith("ARTEMIS")) {
--- End diff --
The link the between the script and the caller seems kind of fragile so I put those checks in there in case the caller was ever changed it wouldn't blow up immediately.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161980690
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -244,22 +262,32 @@ public static ActiveMQTemporaryTopic createTemporaryTopic(String address) {
protected ActiveMQDestination(final String address,
final TYPE type,
final ActiveMQSession session) {
- this.simpleAddress = SimpleString.toSimpleString(address);
-
- this.thetype = type;
-
- this.session = session;
+ this(SimpleString.toSimpleString(address), type, session);
+ }
- this.temporary = TYPE.isTemporary(type);
+ protected ActiveMQDestination(final SimpleString address,
+ final TYPE type,
+ final ActiveMQSession session) {
+ this(address, address != null ? address.toString() : null, type, session);
+ }
- this.queue = TYPE.isQueue(type);
+ protected ActiveMQDestination(final String address,
+ final String name,
+ final TYPE type,
+ final ActiveMQSession session) {
+ this(SimpleString.toSimpleString(address), name, type, session);
}
protected ActiveMQDestination(final SimpleString address,
--- End diff --
e.g.
```
protected ActiveMQDestination(final String address,
final TYPE type,
final ActiveMQSession session) {
this(SimpleString.toSimpleString(address), type, session);
}
protected ActiveMQDestination(final SimpleString address,
final TYPE type,
final ActiveMQSession session) {
setSimpleAddress(address);
this.thetype = type;
this.session = session;
this.temporary = TYPE.isTemporary(type);
this.queue = TYPE.isQueue(type);
}
@Deprecated
protected ActiveMQDestination(final String address,
final String name,
final TYPE type,
final ActiveMQSession session) {
this(SimpleString.toSimpleString(address), name, type, session);
}
@Deprecated
protected ActiveMQDestination(final SimpleString address,
final String name,
final TYPE type,
final ActiveMQSession session) {
this(address, type, session);
this.name = name;
}
```
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161978297
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -309,7 +338,7 @@ public SimpleString getSimpleAddress() {
}
public String getName() {
- return simpleAddress.toString();
+ return name;
--- End diff --
name can be null, if this is the case should use simpleAddress.toString.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r162116374
--- Diff: tests/compatibility-tests/src/test/java/org/apache/activemq/artemis/tests/compatibility/SerializationTest.java ---
@@ -91,15 +91,15 @@ public void afterTest() {
@Test
public void testSerializeFactory() throws Throwable {
File file = serverFolder.newFile("objects.ser");
- callScript(senderClassloader, "serial/serial.groovy", file.getAbsolutePath(), "write");
- callScript(receiverClassloader, "serial/serial.groovy", file.getAbsolutePath(), "read");
+ callScript(senderClassloader, "serial/serial.groovy", file.getAbsolutePath(), "write", sender);
+ callScript(receiverClassloader, "serial/serial.groovy", file.getAbsolutePath(), "read", receiver);
}
@Test
public void testJBMSerializeFactory() throws Throwable {
File file = serverFolder.newFile("objectsjbm.ser");
- callScript(senderClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "write");
- callScript(receiverClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "read");
+ callScript(senderClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "write", sender);
--- End diff --
@jbertram mistake?
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161865850
--- Diff: tests/compatibility-tests/src/main/resources/serial/serial.groovy ---
@@ -24,30 +24,46 @@ import org.apache.activemq.artemis.jms.client.*
file = arg[0]
method = arg[1]
-System.out.println("File::" + file);
+version = arg[2]
+System.out.println("File::" + file)
if (method.equals("write")) {
cf = new ActiveMQConnectionFactory("tcp://localhost:61616?confirmationWindowSize=1048576&blockOnDurableSend=false");
queue = new ActiveMQQueue("queue");
topic = new ActiveMQTopic("topic")
+ if (version.equals("ARTEMIS-SNAPSHOT")) {
+ destination = new ActiveMQDestination("address", "name", ActiveMQDestination.TYPE.DESTINATION, null)
--- End diff --
I told you to call another script as sometimes it fails the compilation. Perhaps this constructor won't be checked until you actually called it.
A lot of times I had to do this it involved classNames that didn't exist.. so I needed to a call an external script.
I will wait the test running to be sure.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161978767
--- Diff: tests/compatibility-tests/src/main/resources/serial/serial.groovy ---
@@ -24,30 +24,40 @@ import org.apache.activemq.artemis.jms.client.*
file = arg[0]
method = arg[1]
-System.out.println("File::" + file);
+version = arg[2]
+System.out.println("File::" + file)
if (method.equals("write")) {
cf = new ActiveMQConnectionFactory("tcp://localhost:61616?confirmationWindowSize=1048576&blockOnDurableSend=false");
queue = new ActiveMQQueue("queue");
topic = new ActiveMQTopic("topic")
+ if (version.equals("ARTEMIS-SNAPSHOT")) {
+ destination = new ActiveMQDestination("address", "name", ActiveMQDestination.TYPE.DESTINATION, null)
--- End diff --
I dont think we should be creating ActiveMQDestination with "name" in 2.X this constructor should be deprecated, the "name" is only for compatibility with older client version.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r162105254
--- Diff: artemis-jms-server/src/main/java/org/apache/activemq/artemis/jms/server/JMSServerManager.java ---
@@ -60,6 +60,24 @@ boolean createQueue(boolean storeConfig,
boolean durable,
String... bindings) throws Exception;
+ /**
+ * Creates a JMS Queue.
+ *
+ * @param queueName The name of the core queue to create
+ * @param jmsQueueName the name of this JMS queue
+ * @param selectorString
+ * @param durable
+ * @return true if the queue is created or if it existed and was added to
+ * the Binding Registry
+ * @throws Exception if problems were encountered creating the queue.
+ */
+ boolean createQueue(boolean storeConfig,
--- End diff --
Fair enough. Thanks for the explination.
---
[GitHub] activemq-artemis issue #1778: ARTEMIS-1609 restore 'name' to ActiveMQDestina...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1778
@michaelandrepearce, I appreciate your reviews. They're consistently valuable.
---
[GitHub] activemq-artemis issue #1778: ARTEMIS-1609 restore 'name' to ActiveMQDestina...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1778
Take a look at the latest push and let me know what you think. I think I've got everything in there!
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161794632
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -273,6 +286,14 @@ public void setAddress(String address) {
setSimpleAddress(SimpleString.toSimpleString(address));
}
+ public void setName(String name) {
--- End diff --
The address setter was added as part of an effort to make Artemis JNDI work properly in Tomcat. I assume it's needed here, but I'll remove it for simplicity's sake and it can be added later if necessary.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r162082757
--- Diff: tests/compatibility-tests/src/main/resources/serial/serial.groovy ---
@@ -24,30 +24,40 @@ import org.apache.activemq.artemis.jms.client.*
file = arg[0]
method = arg[1]
-System.out.println("File::" + file);
+version = arg[2]
+System.out.println("File::" + file)
if (method.equals("write")) {
cf = new ActiveMQConnectionFactory("tcp://localhost:61616?confirmationWindowSize=1048576&blockOnDurableSend=false");
queue = new ActiveMQQueue("queue");
topic = new ActiveMQTopic("topic")
+ if (version.equals("ARTEMIS-SNAPSHOT")) {
+ destination = new ActiveMQDestination("address", "name", ActiveMQDestination.TYPE.DESTINATION, null)
--- End diff --
Verifying compatibility is exactly what this test is for so it's valid for it to be using the deprecated fields.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r162123685
--- Diff: tests/compatibility-tests/src/test/java/org/apache/activemq/artemis/tests/compatibility/SerializationTest.java ---
@@ -91,15 +91,15 @@ public void afterTest() {
@Test
public void testSerializeFactory() throws Throwable {
File file = serverFolder.newFile("objects.ser");
- callScript(senderClassloader, "serial/serial.groovy", file.getAbsolutePath(), "write");
- callScript(receiverClassloader, "serial/serial.groovy", file.getAbsolutePath(), "read");
+ callScript(senderClassloader, "serial/serial.groovy", file.getAbsolutePath(), "write", sender);
+ callScript(receiverClassloader, "serial/serial.groovy", file.getAbsolutePath(), "read", receiver);
}
@Test
public void testJBMSerializeFactory() throws Throwable {
File file = serverFolder.newFile("objectsjbm.ser");
- callScript(senderClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "write");
- callScript(receiverClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "read");
+ callScript(senderClassloader, "serial/jbmserial.groovy", file.getAbsolutePath(), "write", sender);
--- End diff --
@jbertram ignore me.. lol... I read "sender" instead of sender..
I"m getting old I guess
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161866462
--- Diff: tests/compatibility-tests/src/main/resources/serial/serial.groovy ---
@@ -24,30 +24,46 @@ import org.apache.activemq.artemis.jms.client.*
file = arg[0]
method = arg[1]
-System.out.println("File::" + file);
+version = arg[2]
+System.out.println("File::" + file)
if (method.equals("write")) {
cf = new ActiveMQConnectionFactory("tcp://localhost:61616?confirmationWindowSize=1048576&blockOnDurableSend=false");
queue = new ActiveMQQueue("queue");
topic = new ActiveMQTopic("topic")
+ if (version.equals("ARTEMIS-SNAPSHOT")) {
+ destination = new ActiveMQDestination("address", "name", ActiveMQDestination.TYPE.DESTINATION, null)
+ } else if (version.equals("ARTEMIS-155")) {
+ destination = new ActiveMQDestination("address", "name", false, true, null)
+ }
ObjectOutputStream objectOutputStream = new ObjectOutputStream(new FileOutputStream(file));
objectOutputStream.writeObject(cf);
objectOutputStream.writeObject(queue)
objectOutputStream.writeObject(topic)
+ if (version.startsWith("ARTEMIS")) {
--- End diff --
I mean... why not simply doing objectOutputStream.writeObject(destination)
version will always start with ARTEMIS
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by jmesnil <gi...@git.apache.org>.
Github user jmesnil commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161788722
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -273,6 +286,14 @@ public void setAddress(String address) {
setSimpleAddress(SimpleString.toSimpleString(address));
}
+ public void setName(String name) {
--- End diff --
What's the use of these setters?
They should not be used for serialization and I don't see why the name (or address) of an instantiated JMS destination would be changed.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161978935
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQQueue.java ---
@@ -41,6 +41,10 @@ public ActiveMQQueue(final String address) {
super(address, TYPE.QUEUE, null);
}
+ public ActiveMQQueue(final String address, final String name) {
--- End diff --
please mark as deprecated.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161794644
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -309,7 +330,7 @@ public SimpleString getSimpleAddress() {
}
public String getName() {
- return simpleAddress.toString();
+ return name == null ? simpleAddress.toString() : name.toString();
--- End diff --
Fair enough.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161979210
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -244,22 +262,32 @@ public static ActiveMQTemporaryTopic createTemporaryTopic(String address) {
protected ActiveMQDestination(final String address,
final TYPE type,
final ActiveMQSession session) {
- this.simpleAddress = SimpleString.toSimpleString(address);
-
- this.thetype = type;
-
- this.session = session;
+ this(SimpleString.toSimpleString(address), type, session);
+ }
- this.temporary = TYPE.isTemporary(type);
+ protected ActiveMQDestination(final SimpleString address,
+ final TYPE type,
+ final ActiveMQSession session) {
+ this(address, address != null ? address.toString() : null, type, session);
+ }
- this.queue = TYPE.isQueue(type);
+ protected ActiveMQDestination(final String address,
+ final String name,
+ final TYPE type,
+ final ActiveMQSession session) {
+ this(SimpleString.toSimpleString(address), name, type, session);
}
protected ActiveMQDestination(final SimpleString address,
--- End diff --
Can this please be marked as deprecated and the main or super constructor just care for simple address.
---
[GitHub] activemq-artemis pull request #1778: ARTEMIS-1609 restore 'name' to ActiveMQ...
Posted by jmesnil <gi...@git.apache.org>.
Github user jmesnil commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1778#discussion_r161788189
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -244,22 +249,30 @@ public static ActiveMQTemporaryTopic createTemporaryTopic(String address) {
protected ActiveMQDestination(final String address,
final TYPE type,
final ActiveMQSession session) {
- this.simpleAddress = SimpleString.toSimpleString(address);
-
- this.thetype = type;
-
- this.session = session;
+ this(SimpleString.toSimpleString(address), type, session);
+ }
- this.temporary = TYPE.isTemporary(type);
+ protected ActiveMQDestination(final SimpleString address,
+ final TYPE type,
+ final ActiveMQSession session) {
+ this(address, null, type, session);
--- End diff --
should pass `address` instead of `null` for the name
---