Review Board 1.7.22


QPID-3265 Can't subscribe to headers exchange using address (rather than BURL)

Review Request #937 - Created June 20, 2011 and discarded

rajith attapattu
QPID-3265
Reviewers
qpid
qpid
The patch makes the following changes

1. AMQSession_0_10.java
   A default binding is only added if there are no explicit bindings specified via x-bindings

2. BasicMessageConsumer_0_10.java
   When the same destination (Topic) is used to create two different consumers the code creates a copy of the destination to ensure the second consumer gets it's own unique temp queue. When doing so we need to ensure we delete any bindings for the previous temp queue. If we don't remove old bindings and if there were no explicit bindings specified via x-bindings, then the second consumers queue will not be bound due to the logic mentioned in [1]. (Bcos the previous binding is treated as explicit bindings).

3. AddressHelper.java
   The second part of the JIRA covers a different bug - i.e x-binding specified in the link properties are not used if the node type if a queue.
    I added code to read the x-bindings in link props if there are no x-bindings specified in the node props.

4. Modified test cases
    1. To ensure that a default binding is not added when explicit bindings are specified.
    2. To fix an existing test case that relied on a default binding even when x-bindings is specified.
(*) I still need to add (or modify an existing test case) to cover the case where x-bindings are specified in link props when the node type if a queue.
The use cases specified in the JIRA were manually tested in addition to the above mentioned automated test cases.
Posted (June 22, 2011, 1:24 p.m.)

   

  
This doesn't seem to distinguish between link- and node- level bindings(?).
If it will remove only one binding why use an array to collect old bindings? Why not just locate the binding to be removed and remove it? It's not clear from this explanation why there would only be one binding however. In fact I don't really understand what is going on in this bit of code at all. Is this trying to remove the default binding added above in line 1329 of AMQSession_0_10?
Again, does this not mean that you can't distinguish between list and node bindings?
Posted (June 22, 2011, 3:09 p.m.)

   

  
The method createSubscriptionQueue is invoked only when the node type is "exchange".

In that case does it make sense to have x-bindings in the node props ?
  1. It is certainly not disallowed.
Agreed. I initially anticipated that there will be more than one binding for the old queue, but realized it's not possible for this specific case.

I should have changed the code to just locate the single binding and removed it instead of the list.

I will fix this.
  1. Can you also clarify what is being done here and why?
Here the assumption is that x-bindings is only specified in either node or link props but not both.

For Topics IMO it doesn't really make sense to have x-bindings in node props.

Even in the Queue case, I am not sure if thee is a valid case where it makes sense to define x-bindings in both places. It should be either in node props or link props.

What do you think ?
  1. Bindings associated with the node are created/deleted if and when the node is created/deleted; bindings associated with the link are created/deleted when the link is opened/closed.
Posted (June 22, 2011, 4:07 p.m.)

   

  
I have created QPID-3317 to cover this issue. For the purpose of this JIRA I will leave this code as it is to get the current issue resolved and aim to resolve the issue raised in this comment as part of QPID-3317
You mean the removal of the specific binding or the reason behind copying the destination, setting queue name to null and removing the binding for the old queue ?

I assume you are looking for more context on what this is done in MessageConsumer?

This is bcos if the same topic destination is used to create two different consumers we need to ensure they both maintain their own unique temp queue while retaining the rest of attributes (ex subject/bindings/x-subscribe props ..etc) the same.
  1. Seems like there is a mixing up of state that is associated with the address and state associated with the link created from that address(?).
    
    More importantly though, if bindings are removed does that not alter the destination? Is it only the default binding that should be removed (since it will be added back in)? Is it possible that any other link could be removed (e.g. if the link name was specified)? If it is just the default binding that should be removed, could the code not be more explicit?
I have created to QPID-3317 to take care of this.
Posted (June 22, 2011, 4:32 p.m.)

   

  
Yes only the default binding should be removed and I will be modifying the code to locate and remove it instead of having a list  - I guess I mentioned that in my first reply. Sorry if I wasn't clear there.

As to your comment about state related to address and link being mixed up is certainly correct. I actually plan to fix that as part of the work for QPID-3317.

As for named links, it will not get removed. The namedQueue boolean takes care of that. If two consumers use a destination with a named link will get an error as it will be trying to create a subscription on a private queue.

Note subscription queues created based on link props are marked exclusive even if it's overridden using x-declare.
  1. The default binding if present will be the only binding, right? Why will only the default binding have the queue set? If an null queue on the binding will be correctly interpreted as implying the current subscription queue, why is the queue set explicitly for the default binding?