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