You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by michaelandrepearce <gi...@git.apache.org> on 2017/06/13 10:21:32 UTC
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
GitHub user michaelandrepearce opened a pull request:
https://github.com/apache/activemq-artemis/pull/1337
ARTEMIS-1205: AMQP Shared Durable Subscriber incorrect behaviour
Use AcitveMQDestination for subscription naming, fixing and aligning queue naming in the process.
The change is behind a configuration toggle so to avoid causing any breaking changes for uses not expecting.
This is a replacement option, to:
https://github.com/apache/activemq-artemis/pull/1328
That makes it config on the server side for AMQP. Also addressing any concerns about breaking anything as everything is behind a configurable switch.
Based on the discussion here, where is seems this maybe more preferable.
https://github.com/apache/activemq-artemis/pull/1316
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1205-AMQPConfig
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1337.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 #1337
----
commit d7ebc78e8e57eb902fb0fd2d1037d4d6b75daa56
Author: Michael Andre Pearce <mi...@me.com>
Date: 2017-06-13T08:23:33Z
ARTEMIS-1205: AMQP Shared Durable Subscriber incorrect behaviour
Use AcitveMQDestination for subscription naming, fixing and aligning queue naming in the process.
The change is behind a configuration toggle so to avoid causing any breaking changes for uses not expecting.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121783157
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
@@ -733,20 +734,31 @@ private static boolean hasCapabilities(Symbol symbol, Source source) {
return false;
}
- private static String createQueueName(String clientId,
+ private static String createQueueName(boolean useLegacyQueueNaming,
+ String clientId,
String pubId,
boolean shared,
boolean global,
boolean isVolatile) {
- String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
- if (shared) {
- if (queue.contains("|")) {
- queue = queue.split("\\|")[0];
- }
- if (isVolatile) {
- queue = "nonDurable" + "." + queue;
+ if (useLegacyQueueNaming) {
+ String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
--- End diff --
Yes, i was reverting the changes so that all the changes are only in the code block when toggled on to avoid any compatibility issues.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r122049520
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
@@ -733,20 +734,31 @@ private static boolean hasCapabilities(Symbol symbol, Source source) {
return false;
}
- private static String createQueueName(String clientId,
+ private static String createQueueName(boolean useLegacyQueueNaming,
+ String clientId,
String pubId,
boolean shared,
boolean global,
boolean isVolatile) {
- String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
- if (shared) {
- if (queue.contains("|")) {
- queue = queue.split("\\|")[0];
- }
- if (isVolatile) {
- queue = "nonDurable" + "." + queue;
+ if (useLegacyQueueNaming) {
+ String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
--- End diff --
done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
@gemmellr great, thanks I've closed therefor the other PR.
Thanks also for the code for the global, seems to do the job nicely :), updated the change with this.
I've re-factored the tests based on your feedback, just doing the shared and durable tests cases now.
Also I've put back in the global check on the existing queue naming.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
@mtaylor post fixing the miss commit, did a fresh checkout to avoid further commit missing issues, ran the mvn commands as given seems all good now, apologies on this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121734250
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java ---
@@ -74,16 +74,20 @@
private final ProtonProtocolManager protocolManager;
+ private final boolean useLegacyQueueNaming;
--- End diff --
As with the constant, indicating its for subscription queue name behaviour would be a little clearer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
@mtaylor just pushed, i had missed committed the revert on that class, am re-running locally with the commands you gave to avoid your time. Once done will ping again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121783550
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpClientTestSupport.java ---
@@ -155,6 +155,7 @@ protected ActiveMQServer createServer(int port) throws Exception {
server.getConfiguration().setPagingDirectory(server.getConfiguration().getPagingDirectory() + port);
server.getConfiguration().setJMXManagementEnabled(true);
server.getConfiguration().setMessageExpiryScanPeriod(5000);
+ server.getConfiguration().setUseLegacyAMQPQueueNaming(false);
--- End diff --
@gemmellr i really/ideally like to have the tests run with it both on and off, any idea how to do that without a massive copy paste exercise.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
@michaelandrepearce
> I assume you're happier with this approach then? can you confirm this?
Yep, I did comment around this yesterday but I put it on the JIRA as thats where I actually read your question, but I'd overlooked you asked it on one of the other PRs.
https://issues.apache.org/jira/browse/ARTEMIS-1205?focusedCommentId=16048159&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16048159
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121922296
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpClientTestSupport.java ---
@@ -74,6 +79,17 @@
protected MBeanServer mBeanServer = MBeanServerFactory.createMBeanServer();
+ @Parameterized.Parameters(name = "{index}: amqpUseCoreSubscriptionNaming={0}")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ {true}, {false}
--- End diff --
I'm confused by this. Is the effect of adding parameters data here with 'true + false' values, and the related parameters data in JMSSharedConsumerTest etc subclasses with only 'true', not that almost every test will be run twice as often and the specific ones for subscriptions will only test the non-default behaviour? If so it seems like the reverse of what we want on both counts?
Regardless, I also think it would be better all round to avoid parameterizing every test in the suite for this, and instead restrict it to only those that specifically need to change it. For example, see the use of AmqpClientTestSupport#isSecurityEnabled() and how only some specific tests use it to affect the broker config. Something similar could be added for the subscription queue naming, or perhaps the general AmqpClientTestSupport#addConfiguration(server) override could be utilized instead, so that only the tests of interest need override the default behaviour and provide the new config (via test paramterization or otherwise).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1337
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121783205
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java ---
@@ -74,16 +74,20 @@
private final ProtonProtocolManager protocolManager;
+ private final boolean useLegacyQueueNaming;
--- End diff --
sure
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r122049646
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpClientTestSupport.java ---
@@ -74,6 +79,17 @@
protected MBeanServer mBeanServer = MBeanServerFactory.createMBeanServer();
+ @Parameterized.Parameters(name = "{index}: amqpUseCoreSubscriptionNaming={0}")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ {true}, {false}
--- End diff --
done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121783316
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/config/ActiveMQDefaultConfiguration.java ---
@@ -442,6 +442,8 @@ public static String getDefaultHapolicyBackupStrategy() {
// Default period to wait between configuration file checks
public static final long DEFAULT_CONFIGURATION_FILE_REFRESH_PERIOD = 5000;
+ public static final boolean DEFAULT_LEGACY_AMQP_QUEUE_NAMING = true;
--- End diff --
sure
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121733233
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/config/ActiveMQDefaultConfiguration.java ---
@@ -442,6 +442,8 @@ public static String getDefaultHapolicyBackupStrategy() {
// Default period to wait between configuration file checks
public static final long DEFAULT_CONFIGURATION_FILE_REFRESH_PERIOD = 5000;
+ public static final boolean DEFAULT_LEGACY_AMQP_QUEUE_NAMING = true;
--- End diff --
It would be good to mention it is "subscription queues" being affected in the constant name. I'm not sure calling the still-default behaviour 'legacy' makes sense, especially as a future change might not just be to flip the boolean here to false but perhaps instead change both the AMQP and Core naming schemes to something without the existing issues. Perhaps flipping the boolean now and calling it something like "USE_CORE_SUBSCRIPTION_QUEUE_NAMES_FOR_AMQP"?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
@gemmellr Thanks for the feedback on this one Robbie. I think my opinions were reflected on the related PR by other members. This approach looks good to me.
Re: Configuration, we may want to pull out protocol specific properties into their own configuration objects (like we have with HA policies). e.g. <protocols><amqp><...>. But that will require some refactor, which can come later if people feel it's necessary. I think what @michaelandrepearce has done here is fine.
I'll kick off a full test suite run before merging.
Cheers
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121737014
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
@@ -733,20 +734,31 @@ private static boolean hasCapabilities(Symbol symbol, Source source) {
return false;
}
- private static String createQueueName(String clientId,
+ private static String createQueueName(boolean useLegacyQueueNaming,
+ String clientId,
String pubId,
boolean shared,
boolean global,
boolean isVolatile) {
- String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
- if (shared) {
- if (queue.contains("|")) {
- queue = queue.split("\\|")[0];
- }
- if (isVolatile) {
- queue = "nonDurable" + "." + queue;
+ if (useLegacyQueueNaming) {
+ String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
+ if (shared) {
+ if (queue.contains("|")) {
+ queue = queue.split("\\|")[0];
+ }
+ if (isVolatile) {
+ queue += ":shared-volatile";
+ }
+ if (global) {
+ queue += ":global";
+ }
}
+ return queue;
+ } else {
+ final boolean durable = !isVolatile;
+ final String subscriptionName = pubId.contains("|") ? pubId.split("\\|")[0] : pubId;
+ final String clientID = clientId == null || clientId.isEmpty() || global || pubId.endsWith("|global") ? null : clientId;
--- End diff --
As mentioned on the JIRA/other PR, the broker should not be inspecting the link name (here passed as 'pubId' it seems) to see if it ends in "|global" specifically and then acting differently, but instead ensure that the 'global' flag passed is correct.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
Re AmqpClientTestSupport i thought i removed the paramatized from this based on review comment i got (earlier i had this), maybe i missed some bit in my commit, will check.
Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r122049574
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
@@ -733,20 +734,31 @@ private static boolean hasCapabilities(Symbol symbol, Source source) {
return false;
}
- private static String createQueueName(String clientId,
+ private static String createQueueName(boolean useLegacyQueueNaming,
+ String clientId,
String pubId,
boolean shared,
boolean global,
boolean isVolatile) {
- String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
- if (shared) {
- if (queue.contains("|")) {
- queue = queue.split("\\|")[0];
- }
- if (isVolatile) {
- queue = "nonDurable" + "." + queue;
+ if (useLegacyQueueNaming) {
+ String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
+ if (shared) {
+ if (queue.contains("|")) {
+ queue = queue.split("\\|")[0];
+ }
+ if (isVolatile) {
+ queue += ":shared-volatile";
+ }
+ if (global) {
+ queue += ":global";
+ }
}
+ return queue;
+ } else {
+ final boolean durable = !isVolatile;
+ final String subscriptionName = pubId.contains("|") ? pubId.split("\\|")[0] : pubId;
+ final String clientID = clientId == null || clientId.isEmpty() || global || pubId.endsWith("|global") ? null : clientId;
--- End diff --
cheers :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121920556
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
@@ -733,20 +734,31 @@ private static boolean hasCapabilities(Symbol symbol, Source source) {
return false;
}
- private static String createQueueName(String clientId,
+ private static String createQueueName(boolean useLegacyQueueNaming,
+ String clientId,
String pubId,
boolean shared,
boolean global,
boolean isVolatile) {
- String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
- if (shared) {
- if (queue.contains("|")) {
- queue = queue.split("\\|")[0];
- }
- if (isVolatile) {
- queue = "nonDurable" + "." + queue;
+ if (useLegacyQueueNaming) {
+ String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
+ if (shared) {
+ if (queue.contains("|")) {
+ queue = queue.split("\\|")[0];
+ }
+ if (isVolatile) {
+ queue += ":shared-volatile";
+ }
+ if (global) {
+ queue += ":global";
+ }
}
+ return queue;
+ } else {
+ final boolean durable = !isVolatile;
+ final String subscriptionName = pubId.contains("|") ? pubId.split("\\|")[0] : pubId;
+ final String clientID = clientId == null || clientId.isEmpty() || global || pubId.endsWith("|global") ? null : clientId;
--- End diff --
Yep, as I mentioned on the JIRA/other PRs the lack of a Source during unsubscribe is an unfortunate (but unavoidable) complexity of the JMS subscription mapping, and the broker instead needs to look for the 'global' hint in the link desired capabilities the JMS client sent when doing the 'null source lookup' needed during unsubscribe.
Something like this is what I meant:
```
--- a/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java
+++ b/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java
@@ -68,6 +68,7 @@ import org.apache.qpid.proton.amqp.transport.ReceiverSettleMode;
import org.apache.qpid.proton.amqp.transport.SenderSettleMode;
import org.apache.qpid.proton.engine.Delivery;
import org.apache.qpid.proton.engine.EndpointState;
+import org.apache.qpid.proton.engine.Link;
import org.apache.qpid.proton.engine.Sender;
import org.jboss.logging.Logger;
@@ -188,6 +189,8 @@ public class ProtonServerSenderContext extends ProtonInitializable implements Pr
// subscription queue
String clientId = getClientId();
String pubId = sender.getName();
+ global = hasRemoteDesiredCapability(sender, GLOBAL);
+
queue = createQueueName(clientId, pubId, true, global, false);
QueueQueryResult result = sessionSPI.queueQuery(queue, RoutingType.MULTICAST, false);
multicast = true;
@@ -720,6 +723,19 @@ public class ProtonServerSenderContext extends ProtonInitializable implements Pr
}
}
+ private static boolean hasRemoteDesiredCapability(Link link, Symbol capability) {
+ Symbol[] remoteDesiredCapabilities = link.getRemoteDesiredCapabilities();
+ if (remoteDesiredCapabilities != null) {
+ for (Symbol cap : remoteDesiredCapabilities) {
+ if (capability.equals(cap)) {
+ return true;
+ }
+ }
+ }
+
+ return false;
+ }
+
private static boolean hasCapabilities(Symbol symbol, Source source) {
if (source != null) {
if (source.getCapabilities() != null) {
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121737539
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpClientTestSupport.java ---
@@ -155,6 +155,7 @@ protected ActiveMQServer createServer(int port) throws Exception {
server.getConfiguration().setPagingDirectory(server.getConfiguration().getPagingDirectory() + port);
server.getConfiguration().setJMXManagementEnabled(true);
server.getConfiguration().setMessageExpiryScanPeriod(5000);
+ server.getConfiguration().setUseLegacyAMQPQueueNaming(false);
--- End diff --
I don't think the tests should override the default like this, it means none of the tests would be testing the default behaviour of the broker. Explicit tests that verify non-default behaviours would be more appropriate.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121921232
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
@@ -733,20 +734,31 @@ private static boolean hasCapabilities(Symbol symbol, Source source) {
return false;
}
- private static String createQueueName(String clientId,
+ private static String createQueueName(boolean useLegacyQueueNaming,
+ String clientId,
String pubId,
boolean shared,
boolean global,
boolean isVolatile) {
- String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
- if (shared) {
- if (queue.contains("|")) {
- queue = queue.split("\\|")[0];
- }
- if (isVolatile) {
- queue = "nonDurable" + "." + queue;
+ if (useLegacyQueueNaming) {
+ String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
--- End diff --
Ok. I think we should instead leave the fix (checking the global flag and not adding the container-id to the queue name) in place to still resolve the original bug. Its that or raising another PR to put it back in after taking it back out in this one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
Looks good to me, but would be better if folks with more of a clue about the broker and its config etc gave things a look before they merge. @mtaylor, @jbertram ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
@michaelandrepearce There were 46 additional test failures when running the full suite. I've just got around to investigating. Some of them look to be unrelated (we do have a few tests that fail intermittently).
The majority are due to the update to AmqpClientTestSupport. There are a bunch (44) of sub classes that need updating in order to run with Parameterized.class.
e.g. org.apache.activemq.artemis.tests.integration.amqp.AmqpExpiredMessageTest.initializationError
Could you update please. To run the full test suite build the project, then run tests with the test profile.
```
mvn clean install -Pdev -Pextra-tests -DskipTests
mvn test -Ptest
```
Thank you.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121733939
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/client/AMQPClientConnectionFactory.java ---
@@ -39,20 +39,22 @@
private final String containerId;
private final Map<Symbol, Object> connectionProperties;
private final int ttl;
+ private final boolean useLegacyQueueName;
public AMQPClientConnectionFactory(ActiveMQServer server, String containerId, Map<Symbol, Object> connectionProperties, int ttl) {
this.server = server;
this.containerId = containerId;
this.connectionProperties = connectionProperties;
this.ttl = ttl;
+ this.useLegacyQueueName = false;
--- End diff --
I don't know what this class is actually used for, but might it be better to use the same value as the other default?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121735447
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
@@ -733,20 +734,31 @@ private static boolean hasCapabilities(Symbol symbol, Source source) {
return false;
}
- private static String createQueueName(String clientId,
+ private static String createQueueName(boolean useLegacyQueueNaming,
+ String clientId,
String pubId,
boolean shared,
boolean global,
boolean isVolatile) {
- String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
- if (shared) {
- if (queue.contains("|")) {
- queue = queue.split("\\|")[0];
- }
- if (isVolatile) {
- queue = "nonDurable" + "." + queue;
+ if (useLegacyQueueNaming) {
+ String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
--- End diff --
This line looks to be restoring the original bug where the container-id (here being passed as clientId, when really the JMS client has no ClientID, and so uses a 'global subscription') was added to the queue name for global subscriptions when it shouldn't have been?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
@gemmellr thanks for the constructive review comments, ill look to address them, I assume you're happier with this approach then? can you confirm this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121822097
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpClientTestSupport.java ---
@@ -155,6 +155,7 @@ protected ActiveMQServer createServer(int port) throws Exception {
server.getConfiguration().setPagingDirectory(server.getConfiguration().getPagingDirectory() + port);
server.getConfiguration().setJMXManagementEnabled(true);
server.getConfiguration().setMessageExpiryScanPeriod(5000);
+ server.getConfiguration().setUseLegacyAMQPQueueNaming(false);
--- End diff --
forgot theres
> @Parameterized
for this :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
@mtaylor i notice this didn't get merged, is anything outstanding on this i need to address?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121783273
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/client/AMQPClientConnectionFactory.java ---
@@ -39,20 +39,22 @@
private final String containerId;
private final Map<Symbol, Object> connectionProperties;
private final int ttl;
+ private final boolean useLegacyQueueName;
public AMQPClientConnectionFactory(ActiveMQServer server, String containerId, Map<Symbol, Object> connectionProperties, int ttl) {
this.server = server;
this.containerId = containerId;
this.connectionProperties = connectionProperties;
this.ttl = ttl;
+ this.useLegacyQueueName = false;
--- End diff --
Its used just for testing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1337: ARTEMIS-1205: AMQP Shared Durable Subsc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121782960
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
@@ -733,20 +734,31 @@ private static boolean hasCapabilities(Symbol symbol, Source source) {
return false;
}
- private static String createQueueName(String clientId,
+ private static String createQueueName(boolean useLegacyQueueNaming,
+ String clientId,
String pubId,
boolean shared,
boolean global,
boolean isVolatile) {
- String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
- if (shared) {
- if (queue.contains("|")) {
- queue = queue.split("\\|")[0];
- }
- if (isVolatile) {
- queue = "nonDurable" + "." + queue;
+ if (useLegacyQueueNaming) {
+ String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
+ if (shared) {
+ if (queue.contains("|")) {
+ queue = queue.split("\\|")[0];
+ }
+ if (isVolatile) {
+ queue += ":shared-volatile";
+ }
+ if (global) {
+ queue += ":global";
+ }
}
+ return queue;
+ } else {
+ final boolean durable = !isVolatile;
+ final String subscriptionName = pubId.contains("|") ? pubId.split("\\|")[0] : pubId;
+ final String clientID = clientId == null || clientId.isEmpty() || global || pubId.endsWith("|global") ? null : clientId;
--- End diff --
@gemmellr it seems when you do a JMS unsubscribe from the AMQP client, the source is null as such don't have this field, if you could point me to where else we could gain this info in a better fashion?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
@gemmellr hopefully all now addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1337: ARTEMIS-1205: AMQP Shared Durable Subscriber i...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1337
@michaelandrepearce Thanks Michael. I'll do another full suite run here. Once it passes I will merge. Thanks again for the great work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---