[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
GitHub user jbertram opened a pull request:

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

    ARTEMIS-1867 FQQN for producers

   

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-1867-2

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/2434.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 #2434
   
----
commit 3248c7c22b470dc213edaa52320460503ca9d3b3
Author: Justin Bertram <jbertram@...>
Date:   2018-11-10T02:17:39Z

    NO-JIRA de-duplicate createQueue()
   
    There were two different but nearly identical implementations of
    createQueue(). I consolidated these into a single method. There should
    be no semantic differences.

commit 27d7ea59eee90384792c1bd8868715ec44552ede
Author: Justin Bertram <jbertram@...>
Date:   2018-11-12T22:05:53Z

    ARTEMIS-1867 FQQN for producers
   
    There's a *slight* semantic change with the behavior of the queue query
    and binding query to make them consistent with the address query, namely
    that they will return the name of the queue and the name of the address
    in every case and the returned names will be not use the FQQN syntax but
    will be parsed to reflect their actual names in the broker.

----


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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r233744603
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java ---
    @@ -3178,138 +3187,7 @@ public Queue createQueue(final SimpleString address,
                                 final int consumersBeforeDispatch,
                                 final long delayBeforeDispatch,
                                 final boolean autoCreateAddress) throws Exception {
    -      return createQueue(address, routingType, queueName, filterString, user, durable, temporary, ignoreIfExists, transientQueue, autoCreated, maxConsumers, purgeOnNoConsumers, exclusive, lastValue, lastValueKey, nonDestructive, consumersBeforeDispatch, delayBeforeDispatch, autoCreateAddress, false);
    -   }
    -
    -   private Queue createQueue(final SimpleString address,
    --- End diff --
   
    Viewing from a phone, so might be more obvious if using a pc. But where has this method gone with all its logic?


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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r233854063
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java ---
    @@ -3178,138 +3187,7 @@ public Queue createQueue(final SimpleString address,
                                 final int consumersBeforeDispatch,
                                 final long delayBeforeDispatch,
                                 final boolean autoCreateAddress) throws Exception {
    -      return createQueue(address, routingType, queueName, filterString, user, durable, temporary, ignoreIfExists, transientQueue, autoCreated, maxConsumers, purgeOnNoConsumers, exclusive, lastValue, lastValueKey, nonDestructive, consumersBeforeDispatch, delayBeforeDispatch, autoCreateAddress, false);
    -   }
    -
    -   private Queue createQueue(final SimpleString address,
    --- End diff --
   
    There were two different but nearly identical implementations of createQueue(). I consolidated these into a single method. There should be no semantic differences.


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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r234532241
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java ---
    @@ -3178,138 +3187,7 @@ public Queue createQueue(final SimpleString address,
                                 final int consumersBeforeDispatch,
                                 final long delayBeforeDispatch,
                                 final boolean autoCreateAddress) throws Exception {
    -      return createQueue(address, routingType, queueName, filterString, user, durable, temporary, ignoreIfExists, transientQueue, autoCreated, maxConsumers, purgeOnNoConsumers, exclusive, lastValue, lastValueKey, nonDestructive, consumersBeforeDispatch, delayBeforeDispatch, autoCreateAddress, false);
    -   }
    -
    -   private Queue createQueue(final SimpleString address,
    --- End diff --
   
    So afaik, the one being removed is the main one called by bits like config loader etc and invoked more directly by many other code paths, so it would be better to remove the other if any IMO.


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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r234540655
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java ---
    @@ -3178,138 +3187,7 @@ public Queue createQueue(final SimpleString address,
                                 final int consumersBeforeDispatch,
                                 final long delayBeforeDispatch,
                                 final boolean autoCreateAddress) throws Exception {
    -      return createQueue(address, routingType, queueName, filterString, user, durable, temporary, ignoreIfExists, transientQueue, autoCreated, maxConsumers, purgeOnNoConsumers, exclusive, lastValue, lastValueKey, nonDestructive, consumersBeforeDispatch, delayBeforeDispatch, autoCreateAddress, false);
    -   }
    -
    -   private Queue createQueue(final SimpleString address,
    --- End diff --
   
    Thanks, i was going to flag about configurationManaged not being present on the other, but have noticed since you've added it, so it shouldn't be an issue.



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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r239486300
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/BindingsImpl.java ---
    @@ -53,7 +54,13 @@
     
        private final Map<SimpleString, Integer> routingNamePositions = new ConcurrentHashMap<>();
     
    -   private final Map<Long, Binding> bindingsMap = new ConcurrentHashMap<>();
    +   private final Map<Long, Binding> bindingsIdMap = new ConcurrentHashMap<>();
    --- End diff --
   
    Why not using a primitive version of this map? We have somewhere LongConcurrentHashMap (used into the journal too)


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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r239488173
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/BindingsImpl.java ---
    @@ -53,7 +54,13 @@
     
        private final Map<SimpleString, Integer> routingNamePositions = new ConcurrentHashMap<>();
     
    -   private final Map<Long, Binding> bindingsMap = new ConcurrentHashMap<>();
    +   private final Map<Long, Binding> bindingsIdMap = new ConcurrentHashMap<>();
    +
    +   /**
    +    * This is the same as bindingsIdMap but indexed on the binding's uniqueName rather than ID. Two maps are
    +    * maintained to speed routing, otherwise we'd have to loop through the bindingsIdMap when routing to an FQQN.
    +    */
    +   private final Map<SimpleString, Binding> bindingsNameMap = new ConcurrentHashMap<>();
    --- End diff --
   
    I'm not aware of the context here, but  considering that are used 2 different concurrent maps I suppose that there is no chance that data would be different between them from a reader pov right?
    If not I suppose that's fine


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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r239494788
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/BindingsImpl.java ---
    @@ -53,7 +54,13 @@
     
        private final Map<SimpleString, Integer> routingNamePositions = new ConcurrentHashMap<>();
     
    -   private final Map<Long, Binding> bindingsMap = new ConcurrentHashMap<>();
    +   private final Map<Long, Binding> bindingsIdMap = new ConcurrentHashMap<>();
    --- End diff --
   
    What's the advantage of ConcurrentLongHashMap<Binding> over ConcurrentHashMap<Long, Binding>?


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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r239497231
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/BindingsImpl.java ---
    @@ -53,7 +54,13 @@
     
        private final Map<SimpleString, Integer> routingNamePositions = new ConcurrentHashMap<>();
     
    -   private final Map<Long, Binding> bindingsMap = new ConcurrentHashMap<>();
    +   private final Map<Long, Binding> bindingsIdMap = new ConcurrentHashMap<>();
    --- End diff --
   
    It won't use Long instances by just primitive longs, so just footprint: if you know that the map won't be that big i suppose that's fine anyway


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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r239499976
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/BindingsImpl.java ---
    @@ -53,7 +54,13 @@
     
        private final Map<SimpleString, Integer> routingNamePositions = new ConcurrentHashMap<>();
     
    -   private final Map<Long, Binding> bindingsMap = new ConcurrentHashMap<>();
    +   private final Map<Long, Binding> bindingsIdMap = new ConcurrentHashMap<>();
    --- End diff --
   
    Got it. FWIW, I just changed the name of the variable here to differentiate it from the new one I added. Given it's been working fine (as far as we know) so far I'd say it should stay the same and we can change it in another PR if necessary.


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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r239502825
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/BindingsImpl.java ---
    @@ -53,7 +54,13 @@
     
        private final Map<SimpleString, Integer> routingNamePositions = new ConcurrentHashMap<>();
     
    -   private final Map<Long, Binding> bindingsMap = new ConcurrentHashMap<>();
    +   private final Map<Long, Binding> bindingsIdMap = new ConcurrentHashMap<>();
    +
    +   /**
    +    * This is the same as bindingsIdMap but indexed on the binding's uniqueName rather than ID. Two maps are
    +    * maintained to speed routing, otherwise we'd have to loop through the bindingsIdMap when routing to an FQQN.
    +    */
    +   private final Map<SimpleString, Binding> bindingsNameMap = new ConcurrentHashMap<>();
    --- End diff --
   
    Do you mean the same?  I'm not sure I'm following you here.


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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r239505519
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/BindingsImpl.java ---
    @@ -53,7 +54,13 @@
     
        private final Map<SimpleString, Integer> routingNamePositions = new ConcurrentHashMap<>();
     
    -   private final Map<Long, Binding> bindingsMap = new ConcurrentHashMap<>();
    +   private final Map<Long, Binding> bindingsIdMap = new ConcurrentHashMap<>();
    +
    +   /**
    +    * This is the same as bindingsIdMap but indexed on the binding's uniqueName rather than ID. Two maps are
    +    * maintained to speed routing, otherwise we'd have to loop through the bindingsIdMap when routing to an FQQN.
    +    */
    +   private final Map<SimpleString, Binding> bindingsNameMap = new ConcurrentHashMap<>();
    --- End diff --
   
    `bindingsIdMap` and `bindingsNameMap` aren't atomically updated with the same operation because are 2 different maps so I'm expecting that dependently on the timing, querying both could give different results (a `Binding` could be found in one map, but not on the other one).
    It can be avoided if we always query them when both are stable: is it the case?
    I hope to have explained it better what I mean: I'm not aware of the context of usage...


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

[GitHub] activemq-artemis pull request #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2434#discussion_r239530225
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/BindingsImpl.java ---
    @@ -53,7 +54,13 @@
     
        private final Map<SimpleString, Integer> routingNamePositions = new ConcurrentHashMap<>();
     
    -   private final Map<Long, Binding> bindingsMap = new ConcurrentHashMap<>();
    +   private final Map<Long, Binding> bindingsIdMap = new ConcurrentHashMap<>();
    +
    +   /**
    +    * This is the same as bindingsIdMap but indexed on the binding's uniqueName rather than ID. Two maps are
    +    * maintained to speed routing, otherwise we'd have to loop through the bindingsIdMap when routing to an FQQN.
    +    */
    +   private final Map<SimpleString, Binding> bindingsNameMap = new ConcurrentHashMap<>();
    --- End diff --
   
    Both maps are used for routing messages, but they are used for different use-cases which I can't see any overlap between so I don't think there's a problem.


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

[GitHub] activemq-artemis issue #2434: ARTEMIS-1867 FQQN for producers

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2434
 
    From what I can tell everything in this PR is sorted now. I'd like to get this in for the upcoming 2.7.0.


---