Qpid
  1. Qpid
  2. QPID-792

Need to revise QueueBrowser implementation strategy

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: M3
    • Fix Version/s: 0.15
    • Component/s: Java Client
    • Labels:
      None

      Description

      While debuggin and issue I noticed the following behaviour in the AMQQueueBrowser class.

      1) When we create a queue browser we do a subscribe immediately
      followed by a cancel. (And then we subscribe again when we enumerate).
      The rationale for doing so it to verify the selector is valid. This is
      very confusing for customers who look at the log and it is not appropriate IMHO.

      Currently we use client side selectors, so it is easy validate this. The
      above step is completely unnecessary.

      If we are to use server side selectors the right way to do is
      a)Send a subscribe with the selector
      b)Give message credits only when you call the enumerate method.

        Activity

        Hide
        Rajith Attapattu added a comment -

        Thanks for the comments guys. Let me move this conversation to the dev list.

        Rajith

        Show
        Rajith Attapattu added a comment - Thanks for the comments guys. Let me move this conversation to the dev list. Rajith
        Hide
        Robbie Gemmell added a comment -

        True enough. We actually already have queue. and topic. functionality in the JNDI file, though as per QPID-3538 it doesnt work for address strings.

        Show
        Robbie Gemmell added a comment - True enough. We actually already have queue. and topic. functionality in the JNDI file, though as per QPID-3538 it doesnt work for address strings.
        Hide
        Rob Godfrey added a comment -

        I realise forcing users to specify queue or topic in the address string wouldnt be consistent with the other clients, but I do think its worth noting that the Java client isn't entirely consistent with the other clients for obvious reasons and trying to make it more so isnt necessarily always going to be a helpful or useful thing.

        I don't think the queue/topic distinction even needs to go into the address - it should only needs to be defined some way in the JNDI source

        e.g. in a properties file then things that begin

        queue.<NAME>=<address string>
        

        would be queues, and

        topic.<NAME>=<address string>
        

        would be topics

        Show
        Rob Godfrey added a comment - I realise forcing users to specify queue or topic in the address string wouldnt be consistent with the other clients, but I do think its worth noting that the Java client isn't entirely consistent with the other clients for obvious reasons and trying to make it more so isnt necessarily always going to be a helpful or useful thing. I don't think the queue/topic distinction even needs to go into the address - it should only needs to be defined some way in the JNDI source e.g. in a properties file then things that begin queue.<NAME>=<address string> would be queues, and topic.<NAME>=<address string> would be topics
        Hide
        Robbie Gemmell added a comment -

        I'm going to go ahead and accept the changes on this for now, they seem reasonable and very much in the spirit of the JIRA.

        The additional discussion on queue validation should probably move to the dev list as suggested, but I guess I can repost this if it does, so while I'm here...

        I also think that the (Java) client shouldnt be making gueses as to whether something is a Queue or a Topic, as I'm sure was fairly evident from previous mailings on the subject last year. If that questionable behaviour is causing pain, then we should at least consider simply not doing it. Destination is itself only the parent interface of Queue and Topic, it doesnt actually offer any methods (even the toString, though for backwards compatibility reasons admitedly) and really only serves to allow creating Topic and Queue consumers etc without having to have a specific Session type. I realise forcing users to specify queue or topic in the address string wouldnt be consistent with the other clients, but I do think its worth noting that the Java client isn't entirely consistent with the other clients for obvious reasons and trying to make it more so isnt necessarily always going to be a helpful or useful thing.

        Show
        Robbie Gemmell added a comment - I'm going to go ahead and accept the changes on this for now, they seem reasonable and very much in the spirit of the JIRA. The additional discussion on queue validation should probably move to the dev list as suggested, but I guess I can repost this if it does, so while I'm here... I also think that the (Java) client shouldnt be making gueses as to whether something is a Queue or a Topic, as I'm sure was fairly evident from previous mailings on the subject last year. If that questionable behaviour is causing pain, then we should at least consider simply not doing it. Destination is itself only the parent interface of Queue and Topic, it doesnt actually offer any methods (even the toString, though for backwards compatibility reasons admitedly) and really only serves to allow creating Topic and Queue consumers etc without having to have a specific Session type. I realise forcing users to specify queue or topic in the address string wouldnt be consistent with the other clients, but I do think its worth noting that the Java client isn't entirely consistent with the other clients for obvious reasons and trying to make it more so isnt necessarily always going to be a helpful or useful thing.
        Hide
        Rob Godfrey added a comment - - edited

        We should really take this discussion off this JIRA and put it on the dev-list

        However... in answer to

        2. One that tries to resolve an address to see if it's indeed a Queue or a Topic (unless explicitly specified). This is a tricky one and the root cause of the TCK failures.

        I don't think that we should be trying to do this because I'm pretty sure that it is impossible to determine what is a Queue and what is a Topic.

        I think the closest we can come is to say that an address that says you have to create a new temporary auto-delete exclusive queue for every consumer should be treated as a topic... but the converse is not true. As far as I am concerned the distinction between Queue and Topic is something that only the "administrator" can determine, and the code cannot determine dynamically.

        Show
        Rob Godfrey added a comment - - edited We should really take this discussion off this JIRA and put it on the dev-list However... in answer to 2. One that tries to resolve an address to see if it's indeed a Queue or a Topic (unless explicitly specified). This is a tricky one and the root cause of the TCK failures. I don't think that we should be trying to do this because I'm pretty sure that it is impossible to determine what is a Queue and what is a Topic. I think the closest we can come is to say that an address that says you have to create a new temporary auto-delete exclusive queue for every consumer should be treated as a topic... but the converse is not true. As far as I am concerned the distinction between Queue and Topic is something that only the "administrator" can determine, and the code cannot determine dynamically.
        Hide
        Rajith Attapattu added a comment - - edited

        Thanks for sharing your thoughts on this.
        I currently have a patch in our internal branch that does validate the queue when trying to create the QueueBrowser.
        So certainly in line with what you mentioned about checking for the presence of the queue.
        (I also agree with you that creating queues when creating consumers by default is an aberration)

        However the TCK failures are certainly forcing us to look at Queue validation more broadly. Hence my questions.
        It seems to me we need several types of validation.

        1. One that checks for the presence of a queue, which as you mention needs to be performed every time we use the Queue to create something (ex consumers, browsers..etc)

        2. One that tries to resolve an address to see if it's indeed a Queue or a Topic (unless explicitly specified). This is a tricky one and the root cause of the TCK failures.

        3. One that actually acts upon the address instructions to create, assert a Queue and acts on the node and link properties as instructed.

        When I mentioned about validation with jndi or when getQueueName() was invoked, I was thinking more about #2 and,
        When I mentioned "If that's the case then I believe we don't need the validationQueue method here", I actually meant that step #2 should probably have been done before, not inside the QueueBrowser implementation. But in making that point I totally missed the fact that the JMS API expects us to check the presence of the queue as well. So I agree step #1 is indeed needed anytime you create something with a Queue.

        Currently we do all 3 in one method which is not sufficient to satisfy the above cases.

        Rob, here's another interesting question.
        Do we need to do step #3 when we create the QueueBrowser or when we create the actual consumer(s) when getEnumeraiton() is called ?
        That is if we need to create/assert the node, node bindings and link properties/bindings at the time we create the Browser or when we create the actual consumers.

        Show
        Rajith Attapattu added a comment - - edited Thanks for sharing your thoughts on this. I currently have a patch in our internal branch that does validate the queue when trying to create the QueueBrowser. So certainly in line with what you mentioned about checking for the presence of the queue. (I also agree with you that creating queues when creating consumers by default is an aberration) However the TCK failures are certainly forcing us to look at Queue validation more broadly. Hence my questions. It seems to me we need several types of validation. 1. One that checks for the presence of a queue, which as you mention needs to be performed every time we use the Queue to create something (ex consumers, browsers..etc) 2. One that tries to resolve an address to see if it's indeed a Queue or a Topic (unless explicitly specified). This is a tricky one and the root cause of the TCK failures. 3. One that actually acts upon the address instructions to create, assert a Queue and acts on the node and link properties as instructed. When I mentioned about validation with jndi or when getQueueName() was invoked, I was thinking more about #2 and, When I mentioned "If that's the case then I believe we don't need the validationQueue method here", I actually meant that step #2 should probably have been done before, not inside the QueueBrowser implementation. But in making that point I totally missed the fact that the JMS API expects us to check the presence of the queue as well. So I agree step #1 is indeed needed anytime you create something with a Queue. Currently we do all 3 in one method which is not sufficient to satisfy the above cases. Rob, here's another interesting question. Do we need to do step #3 when we create the QueueBrowser or when we create the actual consumer(s) when getEnumeraiton() is called ? That is if we need to create/assert the node, node bindings and link properties/bindings at the time we create the Browser or when we create the actual consumers.
        Hide
        Rob Godfrey added a comment -

        I'm strongly of the opinion that when you create a consumer or a browser you should get an exception if the queue you are trying to consume from / browse does not exist - that is certainly the intent of the JMS API I believe. As such I believe an exception here would be valid (and the fact that historically we have created queues when creating consumers is an aberration).

        Getting the queue out of JNDI doesn't seem an appropriate time to validate as at that point you don;t necessarily have a connection to validate with.

        Show
        Rob Godfrey added a comment - I'm strongly of the opinion that when you create a consumer or a browser you should get an exception if the queue you are trying to consume from / browse does not exist - that is certainly the intent of the JMS API I believe. As such I believe an exception here would be valid (and the fact that historically we have created queues when creating consumers is an aberration). Getting the queue out of JNDI doesn't seem an appropriate time to validate as at that point you don;t necessarily have a connection to validate with.
        Hide
        Rajith Attapattu added a comment -
        + // TODO - should really validate queue exists, but we often rely on creating the consumer to create the queue :(
        + // _session.declareQueuePassive( queue );
        

        Sorry for hijacking this JIRA to raise an issue with Queue validation in general. But this issue is good use case to get some understanding into the more general issue of queue validation.

        Rob, I agree that it would be nice if we can validate the Queue at the time we do session.createQueueBrowser().
        But I'm not sure if we should be validating the queue here or much earlier in piece.
        One issue is that once the destination is looked up, an application can call getQueue() and currently if ADDR is used this returns null.
        (With BURL it's not an issue as the queue name is explicitly given)

        The above is causing numerous TCK failures. So I wonder for Queues, if we should be validating/resolving them at the time it's being retrieved from jndi.
        (This may or may not be possible as we might not have a connection/session in place).

        If that's the case then I believe we don't need the validationQueue method here. For BURL we always create the queue for a consumer so this check becomes unnecessary.

        However a potential issue here is where a Queue that's being created or resolved earlier could have been deleted. But that could happen even between creating the browser and calling getEnumeraiton() or between successive getEumeration() methods. Since we always create/resolve the queue (or create with BURL) when creating a consumer this can be caught and handled.

        Show
        Rajith Attapattu added a comment - + // TODO - should really validate queue exists, but we often rely on creating the consumer to create the queue :( + // _session.declareQueuePassive( queue ); Sorry for hijacking this JIRA to raise an issue with Queue validation in general. But this issue is good use case to get some understanding into the more general issue of queue validation. Rob, I agree that it would be nice if we can validate the Queue at the time we do session.createQueueBrowser(). But I'm not sure if we should be validating the queue here or much earlier in piece. One issue is that once the destination is looked up, an application can call getQueue() and currently if ADDR is used this returns null. (With BURL it's not an issue as the queue name is explicitly given) The above is causing numerous TCK failures. So I wonder for Queues, if we should be validating/resolving them at the time it's being retrieved from jndi. (This may or may not be possible as we might not have a connection/session in place). If that's the case then I believe we don't need the validationQueue method here. For BURL we always create the queue for a consumer so this check becomes unnecessary. However a potential issue here is where a Queue that's being created or resolved earlier could have been deleted. But that could happen even between creating the browser and calling getEnumeraiton() or between successive getEumeration() methods. Since we always create/resolve the queue (or create with BURL) when creating a consumer this can be caught and handled.
        Hide
        Rob Godfrey added a comment -

        Robbie - can you review? thx

        Show
        Rob Godfrey added a comment - Robbie - can you review? thx
        Hide
        Rob Godfrey added a comment -

        AFAICT Now work has been done on this JIRA (AMQQueueBrowser still creates a consumer and closes it to validate selector validity) and therefore it will not be part of M4. Descoping...

        Show
        Rob Godfrey added a comment - AFAICT Now work has been done on this JIRA (AMQQueueBrowser still creates a consumer and closes it to validate selector validity) and therefore it will not be part of M4. Descoping...
        Hide
        Aidan Skinner added a comment -

        Setting component, owner, milestone

        Show
        Aidan Skinner added a comment - Setting component, owner, milestone

          People

          • Assignee:
            Robbie Gemmell
            Reporter:
            Rajith Attapattu
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development