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