> On 2011-06-22 15:09:52, rajith attapattu wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java, > > line 134 > > <https://reviews.apache.org/r/937/diff/1/?file=21364#file21364line134> > > > > 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.
Can you also clarify what is being done here and why? > On 2011-06-22 15:09:52, rajith attapattu wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, > > line 1329 > > <https://reviews.apache.org/r/937/diff/1/?file=21363#file21363line1329> > > > > 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 ? It is certainly not disallowed. > On 2011-06-22 15:09:52, rajith attapattu wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/messaging/address/AddressHelper.java, > > line 255 > > <https://reviews.apache.org/r/937/diff/1/?file=21365#file21365line255> > > > > 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 ? 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. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/937/#review877 ----------------------------------------------------------- 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 > >
