You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by rajith attapattu <ra...@gmail.com> on 2011/10/12 23:09:40 UTC
Re: Review Request: QPID-3401 changes to the core client
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/
-----------------------------------------------------------
(Updated 2011-10-12 21:09:40.553855)
Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
Summary
-------
The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
(Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
In summary the changes are,
1. The code now uses AddressBasedDestination if the syntax is ADDR.
2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
3. The code also delegates creating of subscriptions.
TODO.
1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
(AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
This addresses bug QPID-3401.
https://issues.apache.org/jira/browse/QPID-3401
Diffs
-----
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
Diff: https://reviews.apache.org/r/2364/diff
Testing
-------
All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
Thanks,
rajith
Re: Review Request: QPID-3401 changes to the core client
Posted by rajith attapattu <ra...@gmail.com>.
> On 2011-10-14 13:46:19, Robbie Gemmell wrote:
> > Alex and I have given the 2 posts a look over. This review represents both our thoughts and contains comments on https://reviews.apache.org/r/2364 and https://reviews.apache.org/r/2366
> >
> > The lack of proper testing is for us a barrier to these changes being made so close to the release process beginning, whether the code appeared OK or not. The level of change here is fairly significant to be making this close to the release process without proper confidence through testing, however the current testing does not give that confidence and has thus far proven woefully inadequate. Over 18 months since the current Addressing syntax implementation was added, we are still spotting numerous severe issues relating to its use, none of which are caught by the existing tests and so may or may not still exist in this new/refactored implementation. For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
> >
> > There is a complete lack of unit tests for the Addressing code, both for the current and new implementations. We should be aiming to maximize the amount of unit testing we have, as they are faster to run than system tests, can be much more specific/targeted, and help make it clearer what is and isn't being tested. This must be rectified before commit, not after.
> >
> > That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after. Does that functionality work in the existing implementation, and are there any tests for it?
> >
> > Further to the concerns around testing, the next biggest concern would be the new design itself. For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements.
> >
> > The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist. Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.
> >
> > For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?
> >
> > Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.
> >
> > It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues).
>
> rajith attapattu wrote:
> First up, thanks for taking the time to look at the patches. I appreciate it.
> As for the testing situation I agree that the current suite doesn't adequately cover the area in question. This is a topic that I was hoping to tackle after the release. In fact our current test suite doesn't seem to inspire confidence in many situations. As for the comment on unit testing. I will ensure that there is adequate coverage where is make sense. I will put up a patch for review.
> I also hear your concern about this being done close to the release and have taken that into account in deciding to hold on to it until the trunk is open again.
>
> >>For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
> Could you please provide more details here? Is there a JIRA regarding this? I will investigate this issue and ensure it's covered in the new implementation.
>
> >>That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after.
> Sorry about the confusion. What I meant was that durable subscriptions will not work with this patch and additional work is required. My intention was to post a follow up patch and as a separate review. I held back as there was already two reviews in progress.
> However this patch will only be committed alongside the durable subscription patch.
>
> >>Does that functionality work in the existing implementation, and are there any tests for it?
> It does work with the existing functionality. testDurableSubscriber() in AddressBasedDestinationTest.java
> However we should have destination syntax independent tests so we could leverage the existing durable subscriber tests (all though they themselves are a bit thin).
> In fact in general our test suite should be able to be independent of destination, sessions, connection implementations etc..
>
> I will address your comments related to the design in a separate comment.
>
>
> rajith attapattu wrote:
> Let me discuss your points on the design here. Actually this was one of the goals of putting this up for review. I want to ensure we get our design right.
>
> <quote> For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements. </quote>
>
> I am glad you raised this point. All though not clearly visible with the patch as it doesn't go that far, this is in fact 'the' main goal of the re-factoring.
>
> If you look closely org.apache.qpid.messaging.address.amqp_0_10 contains an <b> 0-10 specific </b> implementation of the <b> address based </b> implementation of a "QpidDestination".
>
> (*) You could also have other syntax based implementations or,
> (*) Address based implementations for various protocol versions.
>
> The rest of the code will be working with QpidDestination interface (or QpidTopic & QpidQueue where necessary) without having to have any knowledge about how the destination was constructed (using a particular syntax) or the finer details of how to create/assert/delete (or even sending and receiving) for a specific protocol version for the respective Destination.
>
> Unfortunately given the time constraints and in order to keep the scope relatively small, I didn't go that far (a fact I clearly mentioned in the description).
> I intend to do this step by step.
>
> Therefore I ended up bridging the new abstraction with the AMQxxx classes via the AddressBasedDestination class. The various AMQxxx classes makes it quite difficult to get to that state without significant work. So I ended up with the bridging mechanism and the dreaded if(BURL)[else(FOO)] for the time being.
>
>
> <quote>The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist.</quote>
>
> I quite disagree with this assessment. The main goal here was to provide a clean abstraction to the rest of the code via the QpidDestination interface without them having to worry about syntax and protocol specifics. It's the underlying "protocol session" that contains the "protocol method" to create/delete queues not really your high level Session implementation. In fact from AMQP 1.0 the protocol session no longer has these methods.
>
> If you make the Destinations fairly dumb then you end up complicating your session implementations.
> Your session implementation will need to know the specifics for a particular syntax (links, nodes ..etc) and also the protocol specifics in order retrieve information from the destination and invoke the protocol specific methods.
>
> If you want to support multiple syntaxes (our current situation) then you end up with the dreaded if (BURL) situation.
> One only needs to look at the AMQSession.java , AMQSession_0_8.java and AMQSession_0_10.java to see why it's a bad idea to have high level session implementations that are protocol specific.
>
> IMO the destinations should be immutable and is smart enough to know what it needs to do and how to do it.
>
> <quote>Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.</quote>
>
> I am not quite sure what you meant here. Could you elaborate more here on how it makes testing difficult? (keeping in mind my above explanation)
>
>
> <quote>It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues). </quote>
>
> IMO Destinations should be immutable once it's created from a string (or more broadly a specification, where the simplest form being a string) !
> If you need to say create a copy, you could do deepCopy() with specific parameters rather than having setters on the Destination.
>
> Creating a durable subscription is a good example here. One reason why I wanted to submit as a separate patch.
> I wanted a way to create the new Topic from the given topic with durability, but not by invoking a setter like setDurable on the newly created Topic.
>
> However I couldn't go that far in a single step due to the way our current code works.
>
> rajith attapattu wrote:
> <quote>Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.</quote>
>
> I agree with you about Destinations being resolved per underlying protocol connection (not the high level JMS destination) as administrative (or other) changes may have made the previous resolution invalid. Infact a Destination should be resolved every time it's being used to create a producer or consumer.
>
> However due to a deadlock issue this was difficult to do in the current code base (see QPID-2808)
<quote>For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?</quote>
Existing code that is not needed are indeed removed. I haven't included them in the patches for review to keep the diff simple. Have a look at the QPID-3401.patch attached to the JIRA to get a sense of all the code that has been removed.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2583
-----------------------------------------------------------
On 2011-10-12 21:09:40, rajith attapattu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2364/
> -----------------------------------------------------------
>
> (Updated 2011-10-12 21:09:40)
>
>
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
>
>
> Summary
> -------
>
> The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
> (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
>
> Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
>
> In summary the changes are,
> 1. The code now uses AddressBasedDestination if the syntax is ADDR.
> 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
> 3. The code also delegates creating of subscriptions.
>
> TODO.
> 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
> 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
>
> (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
>
>
> This addresses bug QPID-3401.
> https://issues.apache.org/jira/browse/QPID-3401
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
>
> Diff: https://reviews.apache.org/r/2364/diff
>
>
> Testing
> -------
>
> All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
>
>
> Thanks,
>
> rajith
>
>
Re: Review Request: QPID-3401 changes to the core client
Posted by rajith attapattu <ra...@gmail.com>.
> On 2011-10-14 13:46:19, Robbie Gemmell wrote:
> > Alex and I have given the 2 posts a look over. This review represents both our thoughts and contains comments on https://reviews.apache.org/r/2364 and https://reviews.apache.org/r/2366
> >
> > The lack of proper testing is for us a barrier to these changes being made so close to the release process beginning, whether the code appeared OK or not. The level of change here is fairly significant to be making this close to the release process without proper confidence through testing, however the current testing does not give that confidence and has thus far proven woefully inadequate. Over 18 months since the current Addressing syntax implementation was added, we are still spotting numerous severe issues relating to its use, none of which are caught by the existing tests and so may or may not still exist in this new/refactored implementation. For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
> >
> > There is a complete lack of unit tests for the Addressing code, both for the current and new implementations. We should be aiming to maximize the amount of unit testing we have, as they are faster to run than system tests, can be much more specific/targeted, and help make it clearer what is and isn't being tested. This must be rectified before commit, not after.
> >
> > That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after. Does that functionality work in the existing implementation, and are there any tests for it?
> >
> > Further to the concerns around testing, the next biggest concern would be the new design itself. For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements.
> >
> > The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist. Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.
> >
> > For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?
> >
> > Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.
> >
> > It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues).
First up, thanks for taking the time to look at the patches. I appreciate it.
As for the testing situation I agree that the current suite doesn't adequately cover the area in question. This is a topic that I was hoping to tackle after the release. In fact our current test suite doesn't seem to inspire confidence in many situations. As for the comment on unit testing. I will ensure that there is adequate coverage where is make sense. I will put up a patch for review.
I also hear your concern about this being done close to the release and have taken that into account in deciding to hold on to it until the trunk is open again.
>>For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
Could you please provide more details here? Is there a JIRA regarding this? I will investigate this issue and ensure it's covered in the new implementation.
>>That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after.
Sorry about the confusion. What I meant was that durable subscriptions will not work with this patch and additional work is required. My intention was to post a follow up patch and as a separate review. I held back as there was already two reviews in progress.
However this patch will only be committed alongside the durable subscription patch.
>>Does that functionality work in the existing implementation, and are there any tests for it?
It does work with the existing functionality. testDurableSubscriber() in AddressBasedDestinationTest.java
However we should have destination syntax independent tests so we could leverage the existing durable subscriber tests (all though they themselves are a bit thin).
In fact in general our test suite should be able to be independent of destination, sessions, connection implementations etc..
I will address your comments related to the design in a separate comment.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2583
-----------------------------------------------------------
On 2011-10-12 21:09:40, rajith attapattu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2364/
> -----------------------------------------------------------
>
> (Updated 2011-10-12 21:09:40)
>
>
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
>
>
> Summary
> -------
>
> The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
> (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
>
> Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
>
> In summary the changes are,
> 1. The code now uses AddressBasedDestination if the syntax is ADDR.
> 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
> 3. The code also delegates creating of subscriptions.
>
> TODO.
> 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
> 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
>
> (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
>
>
> This addresses bug QPID-3401.
> https://issues.apache.org/jira/browse/QPID-3401
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
>
> Diff: https://reviews.apache.org/r/2364/diff
>
>
> Testing
> -------
>
> All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
>
>
> Thanks,
>
> rajith
>
>
Re: Review Request: QPID-3401 changes to the core client
Posted by rajith attapattu <ra...@gmail.com>.
> On 2011-10-14 13:46:19, Robbie Gemmell wrote:
> > Alex and I have given the 2 posts a look over. This review represents both our thoughts and contains comments on https://reviews.apache.org/r/2364 and https://reviews.apache.org/r/2366
> >
> > The lack of proper testing is for us a barrier to these changes being made so close to the release process beginning, whether the code appeared OK or not. The level of change here is fairly significant to be making this close to the release process without proper confidence through testing, however the current testing does not give that confidence and has thus far proven woefully inadequate. Over 18 months since the current Addressing syntax implementation was added, we are still spotting numerous severe issues relating to its use, none of which are caught by the existing tests and so may or may not still exist in this new/refactored implementation. For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
> >
> > There is a complete lack of unit tests for the Addressing code, both for the current and new implementations. We should be aiming to maximize the amount of unit testing we have, as they are faster to run than system tests, can be much more specific/targeted, and help make it clearer what is and isn't being tested. This must be rectified before commit, not after.
> >
> > That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after. Does that functionality work in the existing implementation, and are there any tests for it?
> >
> > Further to the concerns around testing, the next biggest concern would be the new design itself. For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements.
> >
> > The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist. Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.
> >
> > For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?
> >
> > Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.
> >
> > It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues).
>
> rajith attapattu wrote:
> First up, thanks for taking the time to look at the patches. I appreciate it.
> As for the testing situation I agree that the current suite doesn't adequately cover the area in question. This is a topic that I was hoping to tackle after the release. In fact our current test suite doesn't seem to inspire confidence in many situations. As for the comment on unit testing. I will ensure that there is adequate coverage where is make sense. I will put up a patch for review.
> I also hear your concern about this being done close to the release and have taken that into account in deciding to hold on to it until the trunk is open again.
>
> >>For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
> Could you please provide more details here? Is there a JIRA regarding this? I will investigate this issue and ensure it's covered in the new implementation.
>
> >>That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after.
> Sorry about the confusion. What I meant was that durable subscriptions will not work with this patch and additional work is required. My intention was to post a follow up patch and as a separate review. I held back as there was already two reviews in progress.
> However this patch will only be committed alongside the durable subscription patch.
>
> >>Does that functionality work in the existing implementation, and are there any tests for it?
> It does work with the existing functionality. testDurableSubscriber() in AddressBasedDestinationTest.java
> However we should have destination syntax independent tests so we could leverage the existing durable subscriber tests (all though they themselves are a bit thin).
> In fact in general our test suite should be able to be independent of destination, sessions, connection implementations etc..
>
> I will address your comments related to the design in a separate comment.
>
Let me discuss your points on the design here. Actually this was one of the goals of putting this up for review. I want to ensure we get our design right.
<quote> For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements. </quote>
I am glad you raised this point. All though not clearly visible with the patch as it doesn't go that far, this is in fact 'the' main goal of the re-factoring.
If you look closely org.apache.qpid.messaging.address.amqp_0_10 contains an <b> 0-10 specific </b> implementation of the <b> address based </b> implementation of a "QpidDestination".
(*) You could also have other syntax based implementations or,
(*) Address based implementations for various protocol versions.
The rest of the code will be working with QpidDestination interface (or QpidTopic & QpidQueue where necessary) without having to have any knowledge about how the destination was constructed (using a particular syntax) or the finer details of how to create/assert/delete (or even sending and receiving) for a specific protocol version for the respective Destination.
Unfortunately given the time constraints and in order to keep the scope relatively small, I didn't go that far (a fact I clearly mentioned in the description).
I intend to do this step by step.
Therefore I ended up bridging the new abstraction with the AMQxxx classes via the AddressBasedDestination class. The various AMQxxx classes makes it quite difficult to get to that state without significant work. So I ended up with the bridging mechanism and the dreaded if(BURL)[else(FOO)] for the time being.
<quote>The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist.</quote>
I quite disagree with this assessment. The main goal here was to provide a clean abstraction to the rest of the code via the QpidDestination interface without them having to worry about syntax and protocol specifics. It's the underlying "protocol session" that contains the "protocol method" to create/delete queues not really your high level Session implementation. In fact from AMQP 1.0 the protocol session no longer has these methods.
If you make the Destinations fairly dumb then you end up complicating your session implementations.
Your session implementation will need to know the specifics for a particular syntax (links, nodes ..etc) and also the protocol specifics in order retrieve information from the destination and invoke the protocol specific methods.
If you want to support multiple syntaxes (our current situation) then you end up with the dreaded if (BURL) situation.
One only needs to look at the AMQSession.java , AMQSession_0_8.java and AMQSession_0_10.java to see why it's a bad idea to have high level session implementations that are protocol specific.
IMO the destinations should be immutable and is smart enough to know what it needs to do and how to do it.
<quote>Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.</quote>
I am not quite sure what you meant here. Could you elaborate more here on how it makes testing difficult? (keeping in mind my above explanation)
<quote>It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues). </quote>
IMO Destinations should be immutable once it's created from a string (or more broadly a specification, where the simplest form being a string) !
If you need to say create a copy, you could do deepCopy() with specific parameters rather than having setters on the Destination.
Creating a durable subscription is a good example here. One reason why I wanted to submit as a separate patch.
I wanted a way to create the new Topic from the given topic with durability, but not by invoking a setter like setDurable on the newly created Topic.
However I couldn't go that far in a single step due to the way our current code works.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2583
-----------------------------------------------------------
On 2011-10-12 21:09:40, rajith attapattu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2364/
> -----------------------------------------------------------
>
> (Updated 2011-10-12 21:09:40)
>
>
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
>
>
> Summary
> -------
>
> The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
> (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
>
> Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
>
> In summary the changes are,
> 1. The code now uses AddressBasedDestination if the syntax is ADDR.
> 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
> 3. The code also delegates creating of subscriptions.
>
> TODO.
> 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
> 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
>
> (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
>
>
> This addresses bug QPID-3401.
> https://issues.apache.org/jira/browse/QPID-3401
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
>
> Diff: https://reviews.apache.org/r/2364/diff
>
>
> Testing
> -------
>
> All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
>
>
> Thanks,
>
> rajith
>
>
Re: Review Request: QPID-3401 changes to the core client
Posted by rajith attapattu <ra...@gmail.com>.
> On 2011-10-14 13:46:19, Robbie Gemmell wrote:
> > Alex and I have given the 2 posts a look over. This review represents both our thoughts and contains comments on https://reviews.apache.org/r/2364 and https://reviews.apache.org/r/2366
> >
> > The lack of proper testing is for us a barrier to these changes being made so close to the release process beginning, whether the code appeared OK or not. The level of change here is fairly significant to be making this close to the release process without proper confidence through testing, however the current testing does not give that confidence and has thus far proven woefully inadequate. Over 18 months since the current Addressing syntax implementation was added, we are still spotting numerous severe issues relating to its use, none of which are caught by the existing tests and so may or may not still exist in this new/refactored implementation. For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
> >
> > There is a complete lack of unit tests for the Addressing code, both for the current and new implementations. We should be aiming to maximize the amount of unit testing we have, as they are faster to run than system tests, can be much more specific/targeted, and help make it clearer what is and isn't being tested. This must be rectified before commit, not after.
> >
> > That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after. Does that functionality work in the existing implementation, and are there any tests for it?
> >
> > Further to the concerns around testing, the next biggest concern would be the new design itself. For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements.
> >
> > The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist. Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.
> >
> > For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?
> >
> > Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.
> >
> > It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues).
>
> rajith attapattu wrote:
> First up, thanks for taking the time to look at the patches. I appreciate it.
> As for the testing situation I agree that the current suite doesn't adequately cover the area in question. This is a topic that I was hoping to tackle after the release. In fact our current test suite doesn't seem to inspire confidence in many situations. As for the comment on unit testing. I will ensure that there is adequate coverage where is make sense. I will put up a patch for review.
> I also hear your concern about this being done close to the release and have taken that into account in deciding to hold on to it until the trunk is open again.
>
> >>For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
> Could you please provide more details here? Is there a JIRA regarding this? I will investigate this issue and ensure it's covered in the new implementation.
>
> >>That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after.
> Sorry about the confusion. What I meant was that durable subscriptions will not work with this patch and additional work is required. My intention was to post a follow up patch and as a separate review. I held back as there was already two reviews in progress.
> However this patch will only be committed alongside the durable subscription patch.
>
> >>Does that functionality work in the existing implementation, and are there any tests for it?
> It does work with the existing functionality. testDurableSubscriber() in AddressBasedDestinationTest.java
> However we should have destination syntax independent tests so we could leverage the existing durable subscriber tests (all though they themselves are a bit thin).
> In fact in general our test suite should be able to be independent of destination, sessions, connection implementations etc..
>
> I will address your comments related to the design in a separate comment.
>
>
> rajith attapattu wrote:
> Let me discuss your points on the design here. Actually this was one of the goals of putting this up for review. I want to ensure we get our design right.
>
> <quote> For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements. </quote>
>
> I am glad you raised this point. All though not clearly visible with the patch as it doesn't go that far, this is in fact 'the' main goal of the re-factoring.
>
> If you look closely org.apache.qpid.messaging.address.amqp_0_10 contains an <b> 0-10 specific </b> implementation of the <b> address based </b> implementation of a "QpidDestination".
>
> (*) You could also have other syntax based implementations or,
> (*) Address based implementations for various protocol versions.
>
> The rest of the code will be working with QpidDestination interface (or QpidTopic & QpidQueue where necessary) without having to have any knowledge about how the destination was constructed (using a particular syntax) or the finer details of how to create/assert/delete (or even sending and receiving) for a specific protocol version for the respective Destination.
>
> Unfortunately given the time constraints and in order to keep the scope relatively small, I didn't go that far (a fact I clearly mentioned in the description).
> I intend to do this step by step.
>
> Therefore I ended up bridging the new abstraction with the AMQxxx classes via the AddressBasedDestination class. The various AMQxxx classes makes it quite difficult to get to that state without significant work. So I ended up with the bridging mechanism and the dreaded if(BURL)[else(FOO)] for the time being.
>
>
> <quote>The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist.</quote>
>
> I quite disagree with this assessment. The main goal here was to provide a clean abstraction to the rest of the code via the QpidDestination interface without them having to worry about syntax and protocol specifics. It's the underlying "protocol session" that contains the "protocol method" to create/delete queues not really your high level Session implementation. In fact from AMQP 1.0 the protocol session no longer has these methods.
>
> If you make the Destinations fairly dumb then you end up complicating your session implementations.
> Your session implementation will need to know the specifics for a particular syntax (links, nodes ..etc) and also the protocol specifics in order retrieve information from the destination and invoke the protocol specific methods.
>
> If you want to support multiple syntaxes (our current situation) then you end up with the dreaded if (BURL) situation.
> One only needs to look at the AMQSession.java , AMQSession_0_8.java and AMQSession_0_10.java to see why it's a bad idea to have high level session implementations that are protocol specific.
>
> IMO the destinations should be immutable and is smart enough to know what it needs to do and how to do it.
>
> <quote>Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.</quote>
>
> I am not quite sure what you meant here. Could you elaborate more here on how it makes testing difficult? (keeping in mind my above explanation)
>
>
> <quote>It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues). </quote>
>
> IMO Destinations should be immutable once it's created from a string (or more broadly a specification, where the simplest form being a string) !
> If you need to say create a copy, you could do deepCopy() with specific parameters rather than having setters on the Destination.
>
> Creating a durable subscription is a good example here. One reason why I wanted to submit as a separate patch.
> I wanted a way to create the new Topic from the given topic with durability, but not by invoking a setter like setDurable on the newly created Topic.
>
> However I couldn't go that far in a single step due to the way our current code works.
<quote>Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.</quote>
I agree with you about Destinations being resolved per underlying protocol connection (not the high level JMS destination) as administrative (or other) changes may have made the previous resolution invalid. Infact a Destination should be resolved every time it's being used to create a producer or consumer.
However due to a deadlock issue this was difficult to do in the current code base (see QPID-2808)
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2583
-----------------------------------------------------------
On 2011-10-12 21:09:40, rajith attapattu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2364/
> -----------------------------------------------------------
>
> (Updated 2011-10-12 21:09:40)
>
>
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
>
>
> Summary
> -------
>
> The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
> (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
>
> Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
>
> In summary the changes are,
> 1. The code now uses AddressBasedDestination if the syntax is ADDR.
> 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
> 3. The code also delegates creating of subscriptions.
>
> TODO.
> 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
> 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
>
> (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
>
>
> This addresses bug QPID-3401.
> https://issues.apache.org/jira/browse/QPID-3401
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
>
> Diff: https://reviews.apache.org/r/2364/diff
>
>
> Testing
> -------
>
> All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
>
>
> Thanks,
>
> rajith
>
>
Re: Review Request: QPID-3401 changes to the core client
Posted by Robbie Gemmell <ro...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2583
-----------------------------------------------------------
Alex and I have given the 2 posts a look over. This review represents both our thoughts and contains comments on https://reviews.apache.org/r/2364 and https://reviews.apache.org/r/2366
The lack of proper testing is for us a barrier to these changes being made so close to the release process beginning, whether the code appeared OK or not. The level of change here is fairly significant to be making this close to the release process without proper confidence through testing, however the current testing does not give that confidence and has thus far proven woefully inadequate. Over 18 months since the current Addressing syntax implementation was added, we are still spotting numerous severe issues relating to its use, none of which are caught by the existing tests and so may or may not still exist in this new/refactored implementation. For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
There is a complete lack of unit tests for the Addressing code, both for the current and new implementations. We should be aiming to maximize the amount of unit testing we have, as they are faster to run than system tests, can be much more specific/targeted, and help make it clearer what is and isn't being tested. This must be rectified before commit, not after.
That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after. Does that functionality work in the existing implementation, and are there any tests for it?
Further to the concerns around testing, the next biggest concern would be the new design itself. For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements.
The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist. Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.
For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?
Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.
It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues).
- Robbie
On 2011-10-12 21:09:40, rajith attapattu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2364/
> -----------------------------------------------------------
>
> (Updated 2011-10-12 21:09:40)
>
>
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
>
>
> Summary
> -------
>
> The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
> (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
>
> Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
>
> In summary the changes are,
> 1. The code now uses AddressBasedDestination if the syntax is ADDR.
> 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
> 3. The code also delegates creating of subscriptions.
>
> TODO.
> 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
> 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
>
> (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
>
>
> This addresses bug QPID-3401.
> https://issues.apache.org/jira/browse/QPID-3401
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
>
> Diff: https://reviews.apache.org/r/2364/diff
>
>
> Testing
> -------
>
> All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
>
>
> Thanks,
>
> rajith
>
>
Re: Review Request: QPID-3401 changes to the core client
Posted by rajith attapattu <ra...@gmail.com>.
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, line 976
> > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line976>
> >
> > This new isTopic(Destination) call doesnt seem like an improvement. If a destination is a Topic, then it should implement the Topic interface and the existing check is sufficient.
> >
> > Looking at the implementation of the isTopic method in AMQDestination, it seems questionable since its perfectly possible for queues bound to amq.topic not to be getting consumed from as JMS Topics.
>> If a destination is a Topic, then it should implement the Topic interface and the existing check is sufficient.
Rajith: This is actually not true. When a destination is created using a String, you cannot figure out if it's a "Topic" or a "Queue" until it's been resolved.
So we can only return an Object implementing the javax.jms.Destination !
Please note the existing destination implementation is not entirely correct in it's interpretation of the concept of Topics and Queues as it's quite narrow.
Simply bcos a queue is bound to amq.direct does not make it a "Queue' nor does it make it a Topic bcos it's bound to 'amq.topic'.
>> Looking at the implementation of the isTopic method in AMQDestination, it seems questionable since its perfectly possible for queues bound to amq.topic not to be getting consumed from as JMS Topics.
The AddressBasedDestination overrides the isTopic() and isQueue() methods to identify if address resolves to a Queue or a Topic.
What's missing is that if the underlying QpidDestination is null it should resolve it and provide the correct answer.
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, lines 1037-1059
> > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line1037>
> >
> > Is this condition being used as an equivelent to if(BURL)else(ADDR)?
> >
> > This looks like it is in need of some abstraction.
It is infact the same. I used AMQTopic and AddressBasedTopic to make it more explicit.
However I dislike the whole thing as I believe we need a more unified implementation w.r.t Destinations.
As mentioned I didn't go as far I could have, but on the hand I am not sure how far we can go either with the current code base.
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, lines 1110-1121
> > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line1110>
> >
> > Repeated if(AMQTopic)else() conditions, could use abstraction.
> >
> > Can the existing methods for exchangeName and queueName be overriden in the AddressBasedTopic implementation?
I've since done that, but forgot to add it there. The AddressBasedDestination does have these method overriden.
I will update the patch.
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, lines 1250-1284
> > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line1250>
> >
> > Candidate for some sort of Destination factory?
Certainly !
The goal was to have minimal impact on the respective AMQxxx classes during the first phase.
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, line 2515
> > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line2515>
> >
> > The old way seems better. If I add another Topic implementation later on, I'd now have to track this line down and update it.
correct ! The idea was to restrict Topics to the two said implementations to keep it simple until we sort out a proper abstraction at the AMQxxx class level.
I'd expect changes that should make most of the above code obsolete.
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, line 149
> > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line149>
> >
> > This doesnt seem to be used?
correct - forgot to remove this.
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, lines 580-588
> > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line580>
> >
> > Why does this catch an Exception now when it didnt before? Also, Exception rather than something more specific?
The underlying address based implementation could throw an exception here.
As for the exception handling and other changes, I do have further improvements in mind.
This patch is the beginning of a series of improvements I have in mind.
I want to take a step by step approach.
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, line 598
> > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line598>
> >
> > As per previous comments on the isTopic() changes in AMQSession, this condition seems questionable. If a Topic destination wasnt provided, then overriding what it is classed as based on the exchange in use seems invalid. Also, this looks to clash with logic used in BasicMessageConsumer which decides the value of preAcquire in a different way (which is in itself a bug we were already aiming to fix by consolidating those types of decision into 1 place).
This was existing code which I left as it is.
Perhaps I missed your point ?
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, line 1082
> > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line1082>
> >
> > It seems like at least some of these removed methods were better placed where they were.
> >
> > It doesnt seem unreasonable for the protocol-specific implementation of operations depending solely on the session to actually be in the protocol-specific Session implementations. Better here than in the Destination as some of the functionality now appears to be.
I provided an explanation for this in the design discussion part.
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java, line 536
> > <https://reviews.apache.org/r/2364/diff/1/?file=49770#file49770line536>
> >
> > All the casting here seems ugly, and this is an operation ultimately performed by the session so it would seem cleaner if its implementation was ultimately part of the session.
The ugly casting here is due to the lack of a proper abstraction.
I have commented on the above and the specifics about Destinations being smart objects under the design discussion.
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java, line 173
> > <https://reviews.apache.org/r/2364/diff/1/?file=49771#file49771line173>
> >
> > Whitespace error introduced (along with a vast number of others) despite no other change around this code.
Oh dear ! I have not being able to get rid of this issue in eclipse :(
> On 2011-10-14 15:09:04, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java, lines 229-230
> > <https://reviews.apache.org/r/2364/diff/1/?file=49771#file49771line229>
> >
> > TODO ?
Yes, correct. I wanted to achieve the same here without the destination object having to leak address specific details outside of the abstraction.
I wasn't planning to commit with these TODO items and the durable subscription stuff.
The aim was to attach a subsequent patch this weekend and put up what I had for review as early as possible.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2584
-----------------------------------------------------------
On 2011-10-12 21:09:40, rajith attapattu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2364/
> -----------------------------------------------------------
>
> (Updated 2011-10-12 21:09:40)
>
>
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
>
>
> Summary
> -------
>
> The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
> (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
>
> Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
>
> In summary the changes are,
> 1. The code now uses AddressBasedDestination if the syntax is ADDR.
> 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
> 3. The code also delegates creating of subscriptions.
>
> TODO.
> 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
> 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
>
> (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
>
>
> This addresses bug QPID-3401.
> https://issues.apache.org/jira/browse/QPID-3401
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
>
> Diff: https://reviews.apache.org/r/2364/diff
>
>
> Testing
> -------
>
> All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
>
>
> Thanks,
>
> rajith
>
>
Re: Review Request: QPID-3401 changes to the core client
Posted by Robbie Gemmell <ro...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2584
-----------------------------------------------------------
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5792>
This new isTopic(Destination) call doesnt seem like an improvement. If a destination is a Topic, then it should implement the Topic interface and the existing check is sufficient.
Looking at the implementation of the isTopic method in AMQDestination, it seems questionable since its perfectly possible for queues bound to amq.topic not to be getting consumed from as JMS Topics.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5793>
Is this condition being used as an equivelent to if(BURL)else(ADDR)?
This looks like it is in need of some abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5794>
Repeated if(AMQTopic)else() conditions, could use abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5795>
Repeated if(AMQTopic)else() conditions, could use abstraction.
Can the existing methods for exchangeName and queueName be overriden in the AddressBasedTopic implementation?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5796>
Is the Subject used as the queueName for AddressBasedTopics? I thought it was used as the routing key?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5797>
Repeated if(AMQTopic)else() conditions, could use abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5798>
Candidate for some sort of Destination factory?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5799>
Candidate for some sort of Destination factory?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5800>
The old way seems better. If I add another Topic implementation later on, I'd now have to track this line down and update it.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5801>
See earlier comment on this.
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/2364/#comment5802>
This doesnt seem to be used?
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/2364/#comment5803>
Why does this catch an Exception now when it didnt before? Also, Exception rather than something more specific?
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/2364/#comment5807>
The ever-maintainable if-else pattern. Abstraction required.
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/2364/#comment5804>
As per previous comments on the isTopic() changes in AMQSession, this condition seems questionable. If a Topic destination wasnt provided, then overriding what it is classed as based on the exchange in use seems invalid. Also, this looks to clash with logic used in BasicMessageConsumer which decides the value of preAcquire in a different way (which is in itself a bug we were already aiming to fix by consolidating those types of decision into 1 place).
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/2364/#comment5806>
The Destination being an instance of Topic would seem to be a more robust check here. We should always make sure that is true for all our Topic destination Objects.
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/2364/#comment5809>
It seems like at least some of these removed methods were better placed where they were.
It doesnt seem unreasonable for the protocol-specific implementation of operations depending solely on the session to actually be in the protocol-specific Session implementations. Better here than in the Destination as some of the functionality now appears to be.
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/2364/#comment5810>
Yet more if(ADDR), needs abstraction.
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/2364/#comment5811>
As before, why do we now throw an exception, and why do we need to catch all Exceptions here ?
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/2364/#comment5812>
All the casting here seems ugly, and this is an operation ultimately performed by the session so it would seem cleaner if its implementation was ultimately part of the session.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5814>
Part of the ever-present if-else pattern, needs abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5813>
Whitespace error introduced (along with a vast number of others) despite no other change around this code.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5815>
Part of the ever-present if-else pattern, needs abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5816>
TODO ?
- Robbie
On 2011-10-12 21:09:40, rajith attapattu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2364/
> -----------------------------------------------------------
>
> (Updated 2011-10-12 21:09:40)
>
>
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
>
>
> Summary
> -------
>
> The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
> (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
>
> Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
>
> In summary the changes are,
> 1. The code now uses AddressBasedDestination if the syntax is ADDR.
> 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
> 3. The code also delegates creating of subscriptions.
>
> TODO.
> 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
> 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
>
> (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
>
>
> This addresses bug QPID-3401.
> https://issues.apache.org/jira/browse/QPID-3401
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
>
> Diff: https://reviews.apache.org/r/2364/diff
>
>
> Testing
> -------
>
> All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
>
>
> Thanks,
>
> rajith
>
>