[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...

mtaylor
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 <gary.tully@...>
Date:   2018-01-09T12:27:59Z

    [ARTEMIS-1241] check for FQQN and autocreate queue using the configured default routing type

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1759: [ARTEMIS-1241] check for FQQN and autocreate q...

mtaylor
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1759: [ARTEMIS-1241] check for FQQN and autocreate q...

mtaylor
In reply to this post by mtaylor
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?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...

mtaylor
In reply to this post by mtaylor
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...

mtaylor
In reply to this post by mtaylor
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...

mtaylor
In reply to this post by mtaylor
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.  


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...

mtaylor
In reply to this post by mtaylor
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1759: [ARTEMIS-1241] check for FQQN and autoc...

mtaylor
In reply to this post by mtaylor
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1759: [ARTEMIS-1241] check for FQQN and autocreate q...

mtaylor
In reply to this post by mtaylor
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.


---