You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "joshb1050 (via GitHub)" <gi...@apache.org> on 2024/06/12 03:14:01 UTC
[PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
joshb1050 opened a new pull request, #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972
Currently, with 500K+ queues, the cleanup step of TempQueueCleanerUpper requires invoking WildcardAddressManager#getDirectBindings, which is O(k) in the number of queues.
From method profiling, this can consume up to 95% of our CPU time when needing to clean up many of these.
Add a new map to keep track of the direct bindings, and add a test assertion that fails if we don't properly remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1636813616
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -160,15 +172,7 @@ public Collection<Binding> getMatchingBindings(final SimpleString address) throw
@Override
public Collection<Binding> getDirectBindings(final SimpleString address) throws Exception {
SimpleString realAddress = CompositeAddress.extractAddressName(address);
- Collection<Binding> bindings = new ArrayList<>();
-
- nameMap.forEach((bindingUniqueName, bindingAddressPair) -> {
- if (bindingAddressPair.getA().getAddress().equals(realAddress)) {
- bindings.add(bindingAddressPair.getA());
- }
- });
-
- return bindings;
+ return new ArrayList<>(directBindingMap.getOrDefault(realAddress, Collections.emptyList()));
Review Comment:
It seems rather odd to return a new ArrayList that's wrapping a potentially immutable empty list. This should probably be made a bit smarter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1644812334
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -100,6 +104,11 @@ public boolean addBinding(final Binding binding) throws Exception {
if (nameMap.putIfAbsent(binding.getUniqueName(), bindingAddressPair) != null) {
throw ActiveMQMessageBundle.BUNDLE.bindingAlreadyExists(binding);
}
+ directBindingMap.compute(binding.getAddress(), (key, value) -> {
+ Collection<Binding> bindingList = value == null ? new ArrayList<>() : value;
+ bindingList.add(binding);
Review Comment:
Maybe I'm being naive... but as far as I remember.. the ArrayList itself is not supposed to be used from multiple threads.
The compute will do a replay, but you could still call the compute from multiple threads.
I feel like you should have:
```
synchronized(bindingList) {
bindingsList.add(binding);
}
```
Or another data structure to be used here.
similarly on the remove the queue, it should use a synchronized or use a concurrent equivalent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "joshb1050 (via GitHub)" <gi...@apache.org>.
joshb1050 commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1644955884
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -100,6 +104,11 @@ public boolean addBinding(final Binding binding) throws Exception {
if (nameMap.putIfAbsent(binding.getUniqueName(), bindingAddressPair) != null) {
throw ActiveMQMessageBundle.BUNDLE.bindingAlreadyExists(binding);
}
+ directBindingMap.compute(binding.getAddress(), (key, value) -> {
+ Collection<Binding> bindingList = value == null ? new ArrayList<>() : value;
+ bindingList.add(binding);
Review Comment:
The `directBindingMap` is a `ConcurrentHashMap` so it will guarantee thread safety on the `compute` by locking the node. https://stackoverflow.com/a/59601136
Also, this should be tested already via `WildcardAddressManagerUnitTest#testConcurrentCalls`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1645202926
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -100,6 +104,11 @@ public boolean addBinding(final Binding binding) throws Exception {
if (nameMap.putIfAbsent(binding.getUniqueName(), bindingAddressPair) != null) {
throw ActiveMQMessageBundle.BUNDLE.bindingAlreadyExists(binding);
}
+ directBindingMap.compute(binding.getAddress(), (key, value) -> {
+ Collection<Binding> bindingList = value == null ? new ArrayList<>() : value;
+ bindingList.add(binding);
Review Comment:
Yeah.. I did some testing.. I thought the compute would do a loop retry only (I read the wrong code).
I added some testing.. and I changed your code with adding sleeps and everything worked.. I could see the wait to lock threads.
If you could rebase against main now that I added a test to validate this?
also, you can verify check style by doing this:
mvn -Pdev install -DskipTests=true -T2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1639034848
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -100,6 +102,8 @@ public boolean addBinding(final Binding binding) throws Exception {
if (nameMap.putIfAbsent(binding.getUniqueName(), bindingAddressPair) != null) {
throw ActiveMQMessageBundle.BUNDLE.bindingAlreadyExists(binding);
}
+ directBindingMap.computeIfAbsent(binding.getAddress(), (unused) -> new ArrayList<>())
+ .add(binding);
Review Comment:
Using a copy on write structure would seem to be adding a significant amount of GC overhead as each new binding will copy the array and toss out the old instance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "joshb1050 (via GitHub)" <gi...@apache.org>.
joshb1050 commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1639285762
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -116,7 +121,15 @@ public Binding removeBinding(final SimpleString uniqueName, Transaction tx) thro
return null;
}
- removeBindingInternal(binding.getA().getAddress(), uniqueName);
+ SimpleString address = binding.getA().getAddress();
+ removeBindingInternal(address, uniqueName);
+ Collection<Binding> directBindings = directBindingMap.get(address);
+ if (directBindings != null) {
+ directBindings.remove(binding.getA());
+ if (directBindings.isEmpty()) {
Review Comment:
I have now used `compute` which should take advantage of the striped locking nature of the map, and should remove the race condition mentioned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#issuecomment-2177354426
I'm actually going to merge this now...
Thanks for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#issuecomment-2163705211
at least the -Ptests testsuite (complete one) ran fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1638460217
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -100,6 +102,8 @@ public boolean addBinding(final Binding binding) throws Exception {
if (nameMap.putIfAbsent(binding.getUniqueName(), bindingAddressPair) != null) {
throw ActiveMQMessageBundle.BUNDLE.bindingAlreadyExists(binding);
}
+ directBindingMap.computeIfAbsent(binding.getAddress(), (unused) -> new ArrayList<>())
+ .add(binding);
Review Comment:
you are using ConcurrentHashMap on the directBindingsMap, however you're using a non Concurrent list on the value (new ArrayList)
I'm wondering if we shouldn't use something more protective in multi-thread here.
I understand it's unlikely the broker to remove or add a queue on the same address from multiple threads.. but it's not impossible. (say an application with a MultiCast address and applications creating and deleting subscriptions from multiple places).
it would be a rare race, but difficult to track if it ever happened. Shouldn't we use something more protective here.. perhaps CopyOnWrite or anything else?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#issuecomment-2163510246
@joshb1050 there's a complete test suite that you can run with -Ptests.
The CI in Jenkins runs a limited version of the test suite for basic validation.
For more through verification I run the entire test suite but I wouldn't have reousrces for such a long process in the public CI from GitHub.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "joshb1050 (via GitHub)" <gi...@apache.org>.
joshb1050 commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1636884171
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -160,15 +172,7 @@ public Collection<Binding> getMatchingBindings(final SimpleString address) throw
@Override
public Collection<Binding> getDirectBindings(final SimpleString address) throws Exception {
SimpleString realAddress = CompositeAddress.extractAddressName(address);
- Collection<Binding> bindings = new ArrayList<>();
-
- nameMap.forEach((bindingUniqueName, bindingAddressPair) -> {
- if (bindingAddressPair.getA().getAddress().equals(realAddress)) {
- bindings.add(bindingAddressPair.getA());
- }
- });
-
- return bindings;
+ return new ArrayList<>(directBindingMap.getOrDefault(realAddress, Collections.emptyList()));
Review Comment:
It's a copy operation, and works efficiently internally (at least in JDK 17). We could do a null check instead and return a new list if null, but this has to be a defensive copy since the tests will otherwise fail.
```
public ArrayList(Collection<? extends E> c) {
Object[] a = c.toArray();
if ((size = a.length) != 0) {
if (c.getClass() == ArrayList.class) {
elementData = a;
} else {
elementData = Arrays.copyOf(a, size, Object[].class);
}
} else {
// replace with empty array.
elementData = EMPTY_ELEMENTDATA;
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1638460217
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -100,6 +102,8 @@ public boolean addBinding(final Binding binding) throws Exception {
if (nameMap.putIfAbsent(binding.getUniqueName(), bindingAddressPair) != null) {
throw ActiveMQMessageBundle.BUNDLE.bindingAlreadyExists(binding);
}
+ directBindingMap.computeIfAbsent(binding.getAddress(), (unused) -> new ArrayList<>())
+ .add(binding);
Review Comment:
you are using ConcurrentHashMap on the directBindingsMap, however you're using a non Concurrent list on directBindingsMap.
I'm wondering if we shouldn't use something more protective in multi-thread here.
I understand it's unlikely the broker to remove or add a queue on the same address from multiple threads.. but it's not impossible. (say an application with a MultiCast address and applications creating and deleting subscriptions from multiple places).
it would be a rare race, but difficult to track if it ever happened. Shouldn't we use something more protective here.. perhaps CopyOnWrite or anything else?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic merged PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "joshb1050 (via GitHub)" <gi...@apache.org>.
joshb1050 commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1639286235
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -100,6 +102,8 @@ public boolean addBinding(final Binding binding) throws Exception {
if (nameMap.putIfAbsent(binding.getUniqueName(), bindingAddressPair) != null) {
throw ActiveMQMessageBundle.BUNDLE.bindingAlreadyExists(binding);
}
+ directBindingMap.computeIfAbsent(binding.getAddress(), (unused) -> new ArrayList<>())
+ .add(binding);
Review Comment:
Now using an `ArrayList` given that the modifications are happening in a (synchronized) `compute` block.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#issuecomment-2177236106
I think this can be merged.. I will leave it open for a 1 or 2 days if anyone else wants to give some feedback, but this is +1 from me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "joshb1050 (via GitHub)" <gi...@apache.org>.
joshb1050 commented on PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#issuecomment-2163134836
@clebertsuconic I believe I have fixed, though I wasn't able to see all of those test failures when running locally (and it shows me as the CI passing as well).
This is needed because each time one connection disconnects, say, with a few thousand consumers, the `TempQueueCleanerUpper` needs to clean each of these up, and it invokes `getDirectBindings` for each—so then this ends up being `O(k * n)` where `k` is the number of queues on the server and `n` is the number of consumers on the connection. It completely stops the server from functioning, and seems to be a blocker for any setup that has a few hundred thousand plus temp queues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#issuecomment-2162952102
there are a few test failures:
Test / org.apache.activemq.artemis.tests.integration.cli.AddressCommandTest.testForceDeleteAddressWhenExistsQueues
Test / org.apache.activemq.artemis.tests.integration.addressing.TwoWaysRemoveAddressTest.testDeadLock
Test / org.apache.activemq.artemis.tests.integration.ra.ActiveMQMessageHandlerTest.testSimpleMessageReceivedOnQueueManyMessagesAndInterruptTimeout
Test / org.objectweb.jtests.jms.conform.session.TopicSessionTest.testUnsubscribe
Test / org.objectweb.jtests.jms.conform.session.TopicSessionTest.testCreateSubscriber_1
Test / org.objectweb.jtests.jms.conform.session.TopicSessionTest.testCreateSubscriber_2
Test / org.objectweb.jtests.jms.conform.session.TopicSessionTest.testDurableSubscriber
Test / org.objectweb.jtests.jms.conform.session.TopicSessionTest.testCreateDurableSubscriber_1
Test / org.objectweb.jtests.jms.conform.session.TopicSessionTest.testCreateDurableSubscriber_2
Test / org.objectweb.jtests.jms.conform.session.UnifiedSessionTest.testCreateTemporaryTopicOnQueueSession
Test / org.objectweb.jtests.jms.conform.session.UnifiedSessionTest.testCreateQueueOnTopicSession
Test / org.objectweb.jtests.jms.conform.session.UnifiedSessionTest.testCreateBrowserOnTopicSession
Test / org.objectweb.jtests.jms.conform.session.UnifiedSessionTest.testUnsubscribeOnQueueSession
Test / org.objectweb.jtests.jms.conform.session.UnifiedSessionTest.testCreateTemporaryQueueOnTopicSession
Test / org.objectweb.jtests.jms.conform.session.UnifiedSessionTest.testCreateDurableSubscriberOnQueueSession
Test / org.objectweb.jtests.jms.conform.session.UnifiedSessionTest.testCreateDurableConnectionConsumerOnQueueConnection
Test / org.objectweb.jtests.jms.conform.session.UnifiedSessionTest.testCreateTopicOnQueueSession
Test / org.objectweb.jtests.jms.conform.topic.TemporaryTopicTest.testTemporaryTopic
Test / org.apache.activemq.artemis.tests.integration.amqp.connect.AMQPFederationAddressPolicyTest.testFederationHandlesAddressDeletedAndConsumerRecreates
Test / org.apache.activemq.artemis.tests.integration.amqp.connect.AMQPFederationAddressPolicyTest.testRemoteBrokerClosesFederationReceiverAfterAddressRemoved
Test / org.apache.activemq.artemis.tests.integration.cluster.distribution.SimpleSymmetricClusterTest.testDeleteAddress
Test / org.apache.activemq.artemis.tests.integration.cluster.distribution.SymmetricClusterWithDiscoveryTest.testStartStopServers
Why you need this optimization? Are you constantly querying for the destination on a producer or consumer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1644813299
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -100,6 +104,11 @@ public boolean addBinding(final Binding binding) throws Exception {
if (nameMap.putIfAbsent(binding.getUniqueName(), bindingAddressPair) != null) {
throw ActiveMQMessageBundle.BUNDLE.bindingAlreadyExists(binding);
}
+ directBindingMap.compute(binding.getAddress(), (key, value) -> {
+ Collection<Binding> bindingList = value == null ? new ArrayList<>() : value;
+ bindingList.add(binding);
Review Comment:
say you call this from different threads...
perhaps we should add a test having mulitple threads on this call here, and make sure the outcome is as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#issuecomment-2176977615
There are several checkstyle violations that needs addressed here as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "joshb1050 (via GitHub)" <gi...@apache.org>.
joshb1050 commented on PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#issuecomment-2177070523
> There are several checkstyle violations that needs addressed here as well.
Sorry, Checkstyle not working locally for me. Think I have fixed the issues now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#issuecomment-2162952912
Why you need this optimization? are you querying the queue every time you produce?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1639020568
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -116,7 +121,15 @@ public Binding removeBinding(final SimpleString uniqueName, Transaction tx) thro
return null;
}
- removeBindingInternal(binding.getA().getAddress(), uniqueName);
+ SimpleString address = binding.getA().getAddress();
+ removeBindingInternal(address, uniqueName);
+ Collection<Binding> directBindings = directBindingMap.get(address);
+ if (directBindings != null) {
+ directBindings.remove(binding.getA());
+ if (directBindings.isEmpty()) {
Review Comment:
Isn't there a race condition here in that the directBindings collection might report empty here but a new binding could be added between this check and the next call to remove the bindings entry from the Map meaning that the tracked state is now inconsistent with reality?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "joshb1050 (via GitHub)" <gi...@apache.org>.
joshb1050 commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1639023553
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -116,7 +121,15 @@ public Binding removeBinding(final SimpleString uniqueName, Transaction tx) thro
return null;
}
- removeBindingInternal(binding.getA().getAddress(), uniqueName);
+ SimpleString address = binding.getA().getAddress();
+ removeBindingInternal(address, uniqueName);
+ Collection<Binding> directBindings = directBindingMap.get(address);
+ if (directBindings != null) {
+ directBindings.remove(binding.getA());
+ if (directBindings.isEmpty()) {
Review Comment:
Should we perhaps put this in a `ConcurrentHashMap#compute` lambda instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4814 Make direct binding lookup time constant [activemq-artemis]
Posted by "joshb1050 (via GitHub)" <gi...@apache.org>.
joshb1050 commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1638740605
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -100,6 +102,8 @@ public boolean addBinding(final Binding binding) throws Exception {
if (nameMap.putIfAbsent(binding.getUniqueName(), bindingAddressPair) != null) {
throw ActiveMQMessageBundle.BUNDLE.bindingAlreadyExists(binding);
}
+ directBindingMap.computeIfAbsent(binding.getAddress(), (unused) -> new ArrayList<>())
+ .add(binding);
Review Comment:
Good point, definitely should be thread-safe. Changed to a COW ArrayList.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact