----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/937/#review877 -----------------------------------------------------------
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java <https://reviews.apache.org/r/937/#comment1911> 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 ? http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java <https://reviews.apache.org/r/937/#comment1912> 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. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/messaging/address/AddressHelper.java <https://reviews.apache.org/r/937/#comment1913> 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 ? - rajith On 2011-06-20 17:29:56, rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/937/ > ----------------------------------------------------------- > > (Updated 2011-06-20 17:29:56) > > > Review request for qpid. > > > Summary > ------- > > 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. > > > This addresses bug QPID-3265. > https://issues.apache.org/jira/browse/QPID-3265 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java > 1137691 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java > 1137691 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/messaging/address/AddressHelper.java > 1137691 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/client/destination/AddressBasedDestinationTest.java > 1137691 > > Diff: https://reviews.apache.org/r/937/diff > > > Testing > ------- > > The use cases specified in the JIRA were manually tested in addition to the > above mentioned automated test cases. > > > Thanks, > > rajith > >
