[GitHub] activemq-artemis pull request #1316: ARTEMIS-1205: AMQP Shared Durable Subsc...

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

[GitHub] activemq-artemis pull request #1316: ARTEMIS-1205: AMQP Shared Durable Subsc...

asfgit
GitHub user michaelandrepearce opened a pull request:

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

    ARTEMIS-1205: AMQP Shared Durable Subscriber incorrect behaviour.

    Add test case, to prove the issue, and then obviously ensure it works, post fix.
    Apply changes in logic of createQueueName to handle global better and fix the behaviour.

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

    $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1205

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

    https://github.com/apache/activemq-artemis/pull/1316.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 #1316
   
----
commit 9c04144beae9212826b86d519702f111a258dcbe
Author: Michael Andre Pearce <[hidden email]>
Date:   2017-06-05T12:45:25Z

    ARTEMIS-1205: AMQP Shared Durable Subscriber incorrect behaviour.
   
    Add test case, to prove the issue, and then obviously ensure it works, post fix.
    Apply changes in logic of createQueueName to handle global better and fix the behaviour.

----


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1316: ARTEMIS-1205: AMQP Shared Durable Subsc...

asfgit
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1316#discussion_r120104234
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
    @@ -738,17 +738,14 @@ private static String createQueueName(String clientId,
                                              boolean shared,
                                              boolean global,
                                              boolean isVolatile) {
    -      String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
    +      String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
    --- End diff --
   
    👍 ...
    I will run the testsuite and merge 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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1316: ARTEMIS-1205: AMQP Shared Durable Subsc...

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

    https://github.com/apache/activemq-artemis/pull/1316#discussion_r120105956
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
    @@ -738,17 +738,14 @@ private static String createQueueName(String clientId,
                                              boolean shared,
                                              boolean global,
                                              boolean isVolatile) {
    -      String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
    +      String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
    --- End diff --
   
    wait one second I'm just sorting our SharedConsumer also :) , just pushing :)


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1316: ARTEMIS-1205: AMQP Shared Durable Subsc...

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

    https://github.com/apache/activemq-artemis/pull/1316#discussion_r120106685
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
    @@ -738,17 +738,14 @@ private static String createQueueName(String clientId,
                                              boolean shared,
                                              boolean global,
                                              boolean isVolatile) {
    -      String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
    +      String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
    --- End diff --
   
    pushed. :)


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1316: ARTEMIS-1205: AMQP Shared Durable Subsc...

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

    https://github.com/apache/activemq-artemis/pull/1316#discussion_r120130864
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java ---
    @@ -738,17 +738,14 @@ private static String createQueueName(String clientId,
                                              boolean shared,
                                              boolean global,
                                              boolean isVolatile) {
    -      String queue = clientId == null || clientId.isEmpty() ? pubId : clientId + "." + pubId;
    +      String queue = clientId == null || clientId.isEmpty() || global ? pubId : clientId + "." + pubId;
    --- End diff --
   
    And build is green :)


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

Re: [GitHub] activemq-artemis pull request #1316: ARTEMIS-1205: AMQP Shared Durable Subsc...

clebertsuconic
I will want the full test. Just to be safe. Something I do on my box.
Just to be safe.

Will merge the afternoon.

On Mon, Jun 5, 2017 at 11:13 AM michaelandrepearce <[hidden email]>
wrote:

> Github user michaelandrepearce commented on a diff in the pull request:
>
>
> https://github.com/apache/activemq-artemis/pull/1316#discussion_r120130864
>
>     --- Diff:
> artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java
> ---
>     @@ -738,17 +738,14 @@ private static String createQueueName(String
> clientId,
>                                               boolean shared,
>                                               boolean global,
>                                               boolean isVolatile) {
>     -      String queue = clientId == null || clientId.isEmpty() ? pubId :
> clientId + "." + pubId;
>     +      String queue = clientId == null || clientId.isEmpty() || global
> ? pubId : clientId + "." + pubId;
>     --- End diff --
>
>     And build is green :)
>
>
> ---
> 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 [hidden email] or file a JIRA ticket
> with INFRA.
> ---
>
--
Clebert Suconic
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    @clebertsuconic def no worries, entirely makes sense. Was just updating to let you know everything is ready from my POV.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    This introduced a few failures on this test:
   
    org.apache.activemq.artemis.tests.integration.amqp.ClientDefinedMultiConsumerTest


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    Normal failures... because of the queue names have changed.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    @clebertsuconic should be fixed now.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1316: ARTEMIS-1205: AMQP Shared Durable Subsc...

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

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


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    @michaelandrepearce  I'm thinking about reverting this.. this will cause compatibility issues...
   
    Can we fix only the clientID part...
   
   
    if you really want a new queue naming convention to match core and AMQP, we will need a property somewhere to change the semantic



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    But amqp jms shared consumer is functionally broken without 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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    As in it is not breaking anything


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    But on fixing as we won't be breaking anything we are taking the opportunity to align


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    That is what I'm trying to access.  If it's just one case broken I would fix only this case.  But if it's completely broken then yes we use the opportunity to fix the whole thing.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    Yes it was compleatly broken, that what I added the tests for. If you revert and took just the amqp tests I added you'll note they'd fail


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    @michaelandrepearce that was my impression as well.. but then @gemmellr told me that it was only the case of the clientID that was broken.. according fixing this would break everything else from the POV of compatibility.
   
    So, I"m trying to figure out what is the case now 👍


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user gemmellr commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    The no-ClientID shared-subscription behaviour was indeed broken for AMQP JMS clients. Fixing that issue requires ensuring the container-id is not included in their backing queue name by the broker. Changing those queue names in that way wouldnt break existing AMQP JMS no-ClientID shared-subscription usage because they were already broken. All good.
   
    Introducing the alignment changes in addition to fixing the container-id bug does however introduce other issues that were not previously present for AMQP clients (see the JIRA for some examples), and additionally the latest change to fix alignment of "." handling could actually break existing working subscriptions upon upgrade. So I'd say the question is, is the alignment important enough to introduce those issues for AMQP clients and also potentially break some existing subscriptions? I'm not sure it is, but even if so I'm also not sure I'd do it in a minor version, and at a major version change I would probably instead consider changing the Core side.
   
    I'm heading out shortly, so I won't be around for further replies this evening.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    How does it the alignment changes are in the block of code in the if(shared) section which only applies to shared subscribers the part in point that's broken


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
12