[GitHub] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r251086218
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -393,57 +395,18 @@ private void doSendx(ActiveMQDestination destination,
if (defaultDestination == null) {
throw new UnsupportedOperationException("Destination must be
specified on send with an anonymous producer");
}
-
destination = defaultDestination;
- } else {
- if (defaultDestination != null) {
-if (!destination.equals(defaultDestination)) {
- throw new UnsupportedOperationException("Where a default
destination is specified " + "for the sender and a destination is " +
"specified in the arguments to the send, " + "these destinations must be
equal");
-}
+ } else if (defaultDestination != null) {
+ if (!destination.equals(defaultDestination)) {
+throw new UnsupportedOperationException("Where a default
destination is specified " + "for the sender and a destination is " +
"specified in the arguments to the send, " + "these destinations must be
equal");
}
+ } else {
+ session.checkDestination(destination);
address = destination.getSimpleAddress();
Review comment:
Those lines are the reason why I wanted to run a full testsuite before
merging this.
And I think you're right...
I'm adding a commit to be squashed into yours. I will squash it before
merging it, but if you could review it please
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r251045430
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQSession.java
##
@@ -397,6 +380,55 @@ public MessageProducer createProducer(final Destination
destination) throws JMSE
}
}
+ void checkDestination(ActiveMQDestination destination) throws JMSException {
Review comment:
TBH I hate final methods...
I have needed quite a few times to override them on test cases... so I only
reserve the word final where there is a strong "business" case to not let it be
overwritten. but that's jut my opinion.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r251039152
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/QueueAutoCreationTest.java
##
@@ -133,6 +133,36 @@ public void testAutoCreateOnTopic() throws Exception {
Assert.assertTrue(((ActiveMQConnection)connection).containsKnownDestination(addressName));
}
+ @Test //(timeout = 3)
+ // QueueAutoCreationTest was created to validate auto-creation of queues
+ // and this test was added to validate a regression:
https://issues.apache.org/jira/browse/ARTEMIS-2238
+ public void testAutoCreateOnAddressOnly() throws Exception {
Review comment:
@michaelandrepearce I thought by adding your commit they would be fixed? the
only missing point was a test timeout that I'm reverting.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r251038775
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/QueueAutoCreationTest.java
##
@@ -114,7 +114,7 @@ public void testHugeString() throws Exception {
}
- @Test(timeout = 3)
+ @Test //(timeout = 3)
Review comment:
this is debug.. I will revert it.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250808877
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/QueueAutoCreationTest.java
##
@@ -133,6 +133,36 @@ public void testAutoCreateOnTopic() throws Exception {
Assert.assertTrue(((ActiveMQConnection)connection).containsKnownDestination(addressName));
}
+ @Test //(timeout = 3)
+ // QueueAutoCreationTest was created to validate auto-creation of queues
+ // and this test was added to validate a regression:
https://issues.apache.org/jira/browse/ARTEMIS-2238
+ public void testAutoCreateOnAddressOnly() throws Exception {
Review comment:
this is an incomplete test.. I will finish this before merging
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250808742
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,58 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
Review comment:
I don't think it would be an issue.. but I have ammended it...
I will create a test..
let me run tests on it.. please don't merge this.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250800199
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
Review comment:
it would always add the end, but I'm rewriting the logic now
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250777278
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+} else {
+ if (destination.isQueue()) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+ }
+ }
+
+ connection.addKnownDestination(address);
+}
+ } catch (ActiveMQQueueExistsException e) {
+// The queue was created by another client/admin between the query
check and send create queue packet
+ } catch (ActiveMQException e) {
+throw JMSExceptionHelper.convertFromActiveMQException(e);
+ }
+ }
+ }
+
+ private void checkQueue(ActiveMQDestination destination,
+SimpleString address,
+ClientSession clientSession,
+ClientSession.AddressQuery query) throws
ActiveMQException, InvalidDestinationException {
+ if (destination.isQueue() && query.isAutoCreateQueues()) {
Review comment:
@michaelandrepearce I just wnated to show you an idea... but this is wrong
now
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250776970
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+} else {
+ if (destination.isQueue()) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+ }
+ }
+
+ connection.addKnownDestination(address);
+}
+ } catch (ActiveMQQueueExistsException e) {
+// The queue was created by another client/admin between the query
check and send create queue packet
+ } catch (ActiveMQException e) {
+throw JMSExceptionHelper.convertFromActiveMQException(e);
+ }
+ }
+ }
+
+ private void checkQueue(ActiveMQDestination destination,
+SimpleString address,
+ClientSession clientSession,
+ClientSession.AddressQuery query) throws
ActiveMQException, InvalidDestinationException {
+ if (destination.isQueue() && query.isAutoCreateQueues()) {
Review comment:
let me rewrite this
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250776567
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+} else {
+ if (destination.isQueue()) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+ }
+ }
+
+ connection.addKnownDestination(address);
+}
+ } catch (ActiveMQQueueExistsException e) {
+// The queue was created by another client/admin between the query
check and send create queue packet
+ } catch (ActiveMQException e) {
+throw JMSExceptionHelper.convertFromActiveMQException(e);
+ }
+ }
+ }
+
+ private void checkQueue(ActiveMQDestination destination,
+SimpleString address,
+ClientSession clientSession,
+ClientSession.AddressQuery query) throws
ActiveMQException, InvalidDestinationException {
+ if (destination.isQueue() && query.isAutoCreateQueues()) {
Review comment:
I agree with you. although It wasn't part of my original change.. I was just
fixing a bug
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
