Qpid
  1. Qpid
  2. QPID-3265

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

    Details

    • Type: Bug Bug
    • Status: Reopened
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.10
    • Fix Version/s: None
    • Component/s: Java Client
    • Labels:
    • Environment:

      java 0.10 client

      Description

      Creating a receiver for the following address works from python and c++, but not from JMS (using drain example in each case):

      my-headers-exchange; {link:{x-bindings:[{arguments:{'x-match':all,a:b,c:d}}]}}

      The problem with JMS seems to be that though it correctly interprets the arguments and issues a bind with them in it, it issues another bind to the exchange with no arguments that fails with an error.

      Also, the following also doesn't work:

      my-subscription-queue; {create:always, node:{x-declare:{auto-delete:True}}, link:{x-bindings:[{queue:my-subscription-queue, exchange:my-headers-exchange, arguments:{'x-match':all,a:b,c:d}}]}}

      Here the x-bindings in the link don't seem to get interpreted. However if instead they are moved to the node, that works.

        Activity

        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/937/
        -----------------------------------------------------------

        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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/937/ ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/937/#review876
        -----------------------------------------------------------

        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/#comment1910>

        This doesn't seem to distinguish between link- and node- level bindings.

        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/#comment1909>

        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?

        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/#comment1908>

        Again, does this not mean that you can't distinguish between list and node bindings?

        • Gordon

        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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/937/#review876 ----------------------------------------------------------- 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/#comment1910 > This doesn't seem to distinguish between link- and node- level bindings . 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/#comment1909 > 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? 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/#comment1908 > Again, does this not mean that you can't distinguish between list and node bindings? Gordon 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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

        Show
        jiraposter@reviews.apache.org added a comment - 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/937/#review880
        -----------------------------------------------------------

        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/#comment1918>

        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

        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/#comment1920>

        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.

        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/#comment1919>

        I have created to QPID-3317 to take care of this.

        • 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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/937/#review880 ----------------------------------------------------------- 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/#comment1918 > 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 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/#comment1920 > 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. 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/#comment1919 > I have created to QPID-3317 to take care of this. 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-06-22 16:07:41, 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>

        >

        > 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.

        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?

        • Gordon

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/937/#review880
        -----------------------------------------------------------

        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

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-06-22 16:07:41, 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 > > > 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. 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? Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/937/#review880 ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/937/#review882
        -----------------------------------------------------------

        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/#comment1922>

        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.

        • 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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/937/#review882 ----------------------------------------------------------- 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/#comment1922 > 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. 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-06-22 16:32:18, 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>

        >

        > 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.

        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?

        • Gordon

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/937/#review882
        -----------------------------------------------------------

        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

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-06-22 16:32:18, 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 > > > 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. 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? Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/937/#review882 ----------------------------------------------------------- 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
        Hide
        Robbie Gemmell added a comment -

        Closing issue out as part of JIRA cleanup. Issue may already be resolved, may be invalid, or may never be fixed. See QPID-3469 for further details.

        Show
        Robbie Gemmell added a comment - Closing issue out as part of JIRA cleanup. Issue may already be resolved, may be invalid, or may never be fixed. See QPID-3469 for further details.
        Hide
        Robbie Gemmell added a comment -

        Reopening for 0.13, missed the cleanup target with this one I think as it doesnt look like the diffs discussed above have been applied afterall,

        Show
        Robbie Gemmell added a comment - Reopening for 0.13, missed the cleanup target with this one I think as it doesnt look like the diffs discussed above have been applied afterall,
        Hide
        Robbie Gemmell added a comment -

        Updating fix-for to an unreleased version.

        Show
        Robbie Gemmell added a comment - Updating fix-for to an unreleased version.
        Hide
        Robbie Gemmell added a comment -

        Removing the released-version 0.15 fix-for, doenst seem like anything has been done on this except a small commit which says in its log that it doesnt fix the issue....

        Show
        Robbie Gemmell added a comment - Removing the released-version 0.15 fix-for, doenst seem like anything has been done on this except a small commit which says in its log that it doesnt fix the issue....

          People

          • Assignee:
            Rajith Attapattu
            Reporter:
            Gordon Sim
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development