You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by gtully <gi...@git.apache.org> on 2018/01/09 12:44:48 UTC
[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...
GitHub user gtully opened a pull request:
https://github.com/apache/activemq-artemis/pull/1759
[ARTEMIS-1241] check for FQQN and autocreate queue using the configur…
…ed default routing type
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gtully/activemq-artemis ARTEMIS-1241
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1759.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 #1759
----
commit 41e227e7fb2832829606233770b3234d82aa14aa
Author: gtully <ga...@...>
Date: 2018-01-09T12:27:59Z
[ARTEMIS-1241] check for FQQN and autocreate queue using the configured default routing type
----
---
[GitHub] activemq-artemis issue #1759: [ARTEMIS-1241] check for FQQN and autocreate q...
Posted by gtully <gi...@git.apache.org>.
Github user gtully commented on the issue:
https://github.com/apache/activemq-artemis/pull/1759
@mtaylor Pushed an update, with the check in the openwire session the change set is smaller and the new tests with an existing address work as expected.
the prefixing remains and the case of multiple routing types is handled already by returning the first entry in the set which may need a revisit when the prefix is handled.
---
[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...
Posted by gtully <gi...@git.apache.org>.
Github user gtully commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1759#discussion_r160949474
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/openwire/FQQNOpenWireTest.java ---
@@ -315,13 +314,13 @@ public void testVirtualTopicFQQN() throws Exception {
}
@Test
- @Ignore("need to figure auto bindings creation")
public void testVirtualTopicFQQNAutoCreate() throws Exception {
Connection exConn = null;
SimpleString topic = new SimpleString("VirtualTopic.Orders");
SimpleString subscriptionQ = new SimpleString("Consumer.A");
+ // defaults are false via test setUp
--- End diff --
thanks @mtaylor - I see, that getDefaultAddressRoutingType should only be used by auto address creation. The queue routing type needs to be figured out from the address.
---
[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1759#discussion_r162047648
--- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java ---
@@ -194,7 +195,14 @@ private boolean checkAutoCreateQueue(SimpleString queueName, boolean isTemporary
try {
if (!queueBinding.isExists()) {
if (bindingQuery.isAutoCreateQueues()) {
- server.createQueue(queueName, RoutingType.ANYCAST, queueName, null, true, isTemporary);
+ SimpleString queueNameToUse = queueName;
+ SimpleString addressToUse = queueName;
+ if (CompositeAddress.isFullyQualified(queueName.toString())) {
+ CompositeAddress compositeAddress = CompositeAddress.getQueueName(queueName.toString());
+ addressToUse = new SimpleString(compositeAddress.getAddress());
+ queueNameToUse = new SimpleString(compositeAddress.getQueueName());
+ }
--- End diff --
@gtully This looks good, except you also need to check that the address exists and if auto-create addresses is switched on. You can get this information from the bindings query.
---
[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1759#discussion_r160926081
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java ---
@@ -2834,7 +2842,7 @@ public Queue createQueue(final SimpleString address,
throw ActiveMQMessageBundle.BUNDLE.invalidRoutingTypeForAddress(routingType, info.getName().toString(), info.getRoutingTypes());
}
- final QueueConfig queueConfig = queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(routingType).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).build();
+ final QueueConfig queueConfig = queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(routingTypeToUse).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).build();
--- End diff --
The routing type should be decided by the protocol handler and passed in.
---
[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1759#discussion_r160925550
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java ---
@@ -2810,19 +2810,27 @@ public Queue createQueue(final SimpleString address,
final QueueConfig.Builder queueConfigBuilder;
- final SimpleString addressToUse = address == null ? queueName : address;
+ RoutingType routingTypeToUse = (routingType == null ? ActiveMQDefaultConfiguration.getDefaultRoutingType() : routingType);
+ SimpleString queueNameToUse = queueName;
+ SimpleString addressToUse = address == null ? queueName : address;
+ if (CompositeAddress.isFullyQualified(address.toString())) {
+ CompositeAddress compositeAddress = CompositeAddress.getQueueName(address.toString());
+ addressToUse = new SimpleString(compositeAddress.getAddress());
+ queueNameToUse = new SimpleString(compositeAddress.getQueueName());
+ AddressSettings as = getAddressSettingsRepository().getMatch(addressToUse.toString());
+ routingTypeToUse = as.getDefaultAddressRoutingType();
--- End diff --
The routing type needs to come from the address. If the address doesn't exist, then it should be auto-created before the queue is created.
---
[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...
Posted by gtully <gi...@git.apache.org>.
Github user gtully commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1759#discussion_r162117583
--- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java ---
@@ -194,7 +195,14 @@ private boolean checkAutoCreateQueue(SimpleString queueName, boolean isTemporary
try {
if (!queueBinding.isExists()) {
if (bindingQuery.isAutoCreateQueues()) {
- server.createQueue(queueName, RoutingType.ANYCAST, queueName, null, true, isTemporary);
+ SimpleString queueNameToUse = queueName;
+ SimpleString addressToUse = queueName;
+ if (CompositeAddress.isFullyQualified(queueName.toString())) {
+ CompositeAddress compositeAddress = CompositeAddress.getQueueName(queueName.toString());
+ addressToUse = new SimpleString(compositeAddress.getAddress());
+ queueNameToUse = new SimpleString(compositeAddress.getQueueName());
+ }
--- End diff --
adding the logic to figure the routingType as the path of least resistance and the most explicit. The null routing type autocreateaddress case does not work in createQueue - maybe that is an indication that the relevant code can be removed.
---
[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1759#discussion_r162047806
--- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java ---
@@ -194,7 +195,14 @@ private boolean checkAutoCreateQueue(SimpleString queueName, boolean isTemporary
try {
if (!queueBinding.isExists()) {
if (bindingQuery.isAutoCreateQueues()) {
- server.createQueue(queueName, RoutingType.ANYCAST, queueName, null, true, isTemporary);
+ SimpleString queueNameToUse = queueName;
+ SimpleString addressToUse = queueName;
+ if (CompositeAddress.isFullyQualified(queueName.toString())) {
+ CompositeAddress compositeAddress = CompositeAddress.getQueueName(queueName.toString());
+ addressToUse = new SimpleString(compositeAddress.getAddress());
+ queueNameToUse = new SimpleString(compositeAddress.getQueueName());
+ }
--- End diff --
server.createQueue() will create the address if it doesn't already exist.
---
[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1759#discussion_r160945016
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java ---
@@ -2810,19 +2810,27 @@ public Queue createQueue(final SimpleString address,
final QueueConfig.Builder queueConfigBuilder;
- final SimpleString addressToUse = address == null ? queueName : address;
+ RoutingType routingTypeToUse = (routingType == null ? ActiveMQDefaultConfiguration.getDefaultRoutingType() : routingType);
--- End diff --
This logic should live in the OpenWire protocol manager.
---
[GitHub] activemq-artemis issue #1759: [ARTEMIS-1241] check for FQQN and autocreate q...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1759
This looks OK to me. @mtaylor, will you review this since you opened the JIRA?
---
[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1759#discussion_r160945285
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/openwire/FQQNOpenWireTest.java ---
@@ -315,13 +314,13 @@ public void testVirtualTopicFQQN() throws Exception {
}
@Test
- @Ignore("need to figure auto bindings creation")
public void testVirtualTopicFQQNAutoCreate() throws Exception {
Connection exConn = null;
SimpleString topic = new SimpleString("VirtualTopic.Orders");
SimpleString subscriptionQ = new SimpleString("Consumer.A");
+ // defaults are false via test setUp
--- End diff --
We should test the case where an address already exists and the default type is ANYCAST.
---
[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1759
---
[GitHub] activemq-artemis issue #1759: [ARTEMIS-1241] check for FQQN and autocreate q...
Posted by gtully <gi...@git.apache.org>.
Github user gtully commented on the issue:
https://github.com/apache/activemq-artemis/pull/1759
I added to some existing auto create address logic in the code path which suffices for my test case. It does appear a little hacky to have three xxToUse local variables and the toString messing on simpleString does not look right.
One thought was to pass a null routingtype to this method but it is not clear to me where that decision should be made. Pulling the value from config for the auto creation case does make sense however.
Please review and maybe it is time to consider pulling all of the auto creation logic into one place.
---
[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...
Posted by gtully <gi...@git.apache.org>.
Github user gtully commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1759#discussion_r162113988
--- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java ---
@@ -194,7 +195,14 @@ private boolean checkAutoCreateQueue(SimpleString queueName, boolean isTemporary
try {
if (!queueBinding.isExists()) {
if (bindingQuery.isAutoCreateQueues()) {
- server.createQueue(queueName, RoutingType.ANYCAST, queueName, null, true, isTemporary);
+ SimpleString queueNameToUse = queueName;
+ SimpleString addressToUse = queueName;
+ if (CompositeAddress.isFullyQualified(queueName.toString())) {
+ CompositeAddress compositeAddress = CompositeAddress.getQueueName(queueName.toString());
+ addressToUse = new SimpleString(compositeAddress.getAddress());
+ queueNameToUse = new SimpleString(compositeAddress.getQueueName());
+ }
--- End diff --
hmm, all the tests had a createProducer earlier that auto created the address.
With a createConsumer first I see the problem, the bindingQuery AddressInfo is null.
Passing a null routing type in that case and some tidy up in auto address creation in serverimpl works but the routingType comes from defaultConfiguration rather than the address settings in that case.
Question - should the caller figure the routingType from the addressSettings and if so should the logic for null routingType in createQueue get whacked?
---