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