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

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

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

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

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

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

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

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

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

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

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

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


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

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

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


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

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

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


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

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

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


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

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

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


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

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

RaiSaurabh
In reply to this post by RaiSaurabh
Github user asfgit closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/1759


---