[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 issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    Agreed the "." Second pr could


---
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
 
    Ok what I propose is, we keep the changes that were in this pr that only affect shared subscribers (broken) and we rework the alignment of the escaped "." In the other to actually maybe in the Artemis client not to escape (but make it a feature flag change)
   
    Is this reasonable?


---
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
 
    For us the alignment is fairly critical, as we will have shared core and amqp env


---
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 this sound to you clebert?


---
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
 
    as long as the default is the current one, and the flag only applies to the client.. than I'm fine...
   
    I would prefer leaving as is.. just one less moving part.. but if that's critical for you I'm fine.. as Long as it doesn't get too bloated in the code. (hard to figure out)...
   
   
    So, if you create a connection factory property.. it's probably ok.. defaulted to what there is 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 michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    Sure will do 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 clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    ```
    For us the alignment is fairly critical, as we will have shared core and amqp env
    ```
   
    Are you sure you need this in production? or that's a testcase only.
   
   
    anyways.. I"m out for 2 weeks starting tomorrow... I'm trying to get a shock treatment to get away from the iphone as much as I can. :)



---
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
 
    https://github.com/apache/activemq-artemis/pull/1326 raised this to revert the code to just what we agreed. It is critical for us as we have some apps/clients that will be sharing subscriptions using amqp and core client, like wise we will be configuring up front the queues for multicast, and it makes it very tricky/almost asking for a disaster for the operations guys to have two differing formats.


---
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
 
    I will most probably won't get the chance tonight to do the other bit, as I'm already beer in hand readying myself for a long weekend in the sun too :)  
   
    If you could maybe tee up with @jbertram to look at that for me when i raise next week if you'll be away.


---
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
 
    After looking to revert this. I came back across another reason why we had to do this.
   
    There still is a fundamental problem with leaving :global in as it stands (so this bit to make it meet spec i can't remove).
   
    We should note, as per JMS Spec:http://docs.oracle.com/javaee/7/api/javax/jms/Session.html#unsubscribe-java.lang.String-
   
    "
    A shared durable subscription and an unshared durable subscription may not have the same name and client identifier (if set). If an unshared durable subscription already exists with the same name and client identifier (if set) then a JMSException is thrown.
    "
   
    This means the current behaviour in AMQP here is wrong, as shared and unshared with the same subscription name are not making the same queue as such above is invalid.
   
    e.g. if clientId = myClient;
   
    createSharedDurableConsumer(myTopic, "mySub")
    will create myClient.mySub:global
   
    createDurableConsumer(myTopic, "mySub")
    will create myClient.mySub
   
    Essentially this is why we need to remove the :global part still in the shared part, otherwise this is breaking spec. The other option would be to add :global (or a rename of :global to :durable) to the durable consumer, but that would break things historically.
   
    As removing :global in shared wouldn't break anything already historically, as SharedDurable is not in a working state. this is why we do have to do 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
 
    Re concerns on the overlap of a shared consumer queue and another queue space, it is already possible to make a jms queue named myClient.mySub:global as such we already have collision issue since 2.0. This change does not increase this risk/problem. I think it is something that users will have to be aware of and just configure/ensure they don't do things that risk that (as it is already an issue!)


---
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
 
    Given that all the queues exist in the same ultimate namespace, there will obviously always be potential for collisions unless 'regular queues' had some name mangling applied too (which is undesirable, and was removedin 2.x because of it). I don't dispute this, I just think it is a question of severity and whether one subscription type by default will/could easily collide with another as well as entirely unrelated things such as regular queues with simple names.
   
    Someone with a queue called "foo", and a topic called "bar" and a client who makes a no-ClientID subscription named "foo", should be allowed to. With the existing format (minus the container-id bug) they would be able to on the AMQP side. With the aligned one they will not as the queue and subscription names would be directly used the same way without attempt to avoid collision in use of these essentially unrelated values.
   
    The addition of a ":global" suffix was not incorrect, the implementation simply had a related bug in it. The quoted JMS spec excerpt (I assume is from createSharedDurableConsumer, not the linked unsubscribe) and example are not actually applicable to it, as the ":global" suffix only applies for shared subscriptions on connections without a ClientID, which cant have non-shared durable subscriptions (i.e they need a ClientID to be set). The ":global" suffix exists precisely to separate those 'global sub backing' queue names from being named directly the same way as regular queues and other sub types when presented with simple name "foo". If you go to the effort of naming all your regular queues "foo:global" etc then I agree you can still cause the same collisions, but I think at that point its actually reasonble to say there is a naming convention and you need to take it into account, as its far less likely to be hit and the default for every simple name "foo" was not for them to c
 ollide.
   
    The ":shared-volatile" and ":global" suffixes also help prevent a durable [shared] subscription backing queue name from easily colliding with a non-durable one, or with those from connections with ClientID's. Removing those with the alignment, that then becomes somewhat more likely, unless the "." escaping is done, which as discussed could break existing subscriptions. Obviously, use of regular queues could again be made to collide names with any naming convention used here, but again for me the key bit is typically only with some deliberate effort.
   
    The issue noted with unsubscribe (which I was planning to test+fix after this JIRA was complete) will most likely be that the broker is not looking in the correct place for the "global" flag as it applies during unsubscribe. This is unfortunately in a different place than it is the rest of the time due to how the complicated AMQP<->JMS mapping must work for shared subs. Typically the flag is added as an AMQP 'source capability', but during unsubscribe the is no 'source' information present so instead it gets passed as a 'link desired capability'. The change to add the below would actually work for the JMS client, but would typically fail/be-incorrect for any non-JMS clients using the mechanism. Whats needed is inspecting the link capabilities and passing the correct global value into the naming method.
    ```
    +      if (pubId.endsWith("|global")) {
    +         global = true;
    +      }
    ```


---
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
 
    I seem to be the only one that considers the direct collision of simple 'regular queue' names and 'no-Client shared subscription backing queue' names to be an issue. If everyone else is happy I'm not standing in the way. I just feel the need to point it out as I think it is a fairly annoying usability issue that shouldnt exist. Personally I'd change both the Core and AMQP sides together in a later 3.0.0 release.
   
    If the alignment is done then I'd possibly go ahead and align the AMQP "." handling behaviour as well, even though it could break some existing subscriptions (hopefully there aren't [m]any affected), given other alignment issues would actually be lessened by it and if the aim is alignment then it would make sense they actually do 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 tabish121 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1316
 
    I'm in agreement with Robbie on this, I think the ease of which the subscription names and "regular queue" names can collide should be avoided.  It's much easier to tell people (document) how to avoid this collision if we have in place some means of at least making it something you couldn't do without putting some deliberate effort into it or have some amazingly bad luck on your side.  
   
    Lining up Core and AMQP mapping names for subscriptions seems like a good thing to add to a 3.0 task list for changes that require a major version bump.  In the meantime if some configuration switch is added to control using one naming scheme or another it ought to be added as an already deprecated sort of thing that is going away in the next major version.  


---
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
 
    Hi,
    I have raised 2 PR's
   
    one is to continue/complete on the above work and align using CORE client side configurable, based on @clebertsuconic comment that he's happy to have that client side in core.
    https://github.com/apache/activemq-artemis/pull/1328
   
    one is to move this logic and to align in the AMQP protocol server side configurable. Based on the last two comments that seems having it configurable so that its configurable if to break existing behaviour, but having the fixes or chose to remain as is (default)
    https://github.com/apache/activemq-artemis/pull/1337
   
    The idea being we chose the more preferable solution out of the two.
   
    I am guessing @gemmellr the second (1337) is your more preferable one. Is every one agreed?


---
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 issue #1316: ARTEMIS-1205: AMQP Shared Durable Subscriber i...

Thank you 123
In reply to this post by asfgit
Hi,

So i have gone through the issue. my question: so how do we specify the that
a address with 2 queues are using shared subscription ? Does it need a
client_id specified or not. where do we get god documentation on artemis
2.10 which has some examples kind of things and all.
TIA




--
Sent from: http://activemq.2283324.n4.nabble.com/ActiveMQ-Dev-f2368404.html
12