----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7412/#review12215 -----------------------------------------------------------
I have reviewed this mostly on a 'its just code' basis, because Im not sure I really know precisely what its actually meant to do (after reading this review request, the original JIRA, and our documentation on addresses). I dont see the mentioned unit tests, did you miss out the relevant file? 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/7412/#comment25897> You can always put //TODO: delete to let your IDE remind you to remove stuff like 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/7412/#comment25899> If its going to be a durable queue should it have a name called TempQueue? What are the ways such a combination could occur? 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/7412/#comment25898> Do we need to add it if it is false? 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/7412/#comment25901> Might as well fix the method names while you are in here. I think someone maybe even raised a JIRA about it recently. 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/7412/#comment25900> Might as well fix the method names while you are in here. I think someone maybe evne raised a JIRA about it recently. 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/7412/#comment25902> Should we add it if its false? 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/7412/#comment25905> This is a little confusing. If they are using Queues, which usually go through the nameless/default exchange, why would we bind to amq.topic? How would things get sent to the broker that would actually use this binding rather than use the queue name on the nameless/default exchange, do people magically need to know to send to amq.topic? (Also, the variable name is a little weird, which is why I use the contrived nameless/default format I did above for the default exchange) 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/7412/#comment25906> We do this in two places (again below), might be worth a method extraction if only so they are consistent. 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/7412/#comment25907> Missing if(_logger.isDebugEnabled). 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/7412/#comment25903> Sincerely, your friend //TODO 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/7412/#comment25908> Message is wrong (clearly a C&P error from the above method. Given how these are almost identical loops, perhaps a method is in order with a boolean paramter to decide between create and delete). Also missing if(_logger.isDebugEnabled) by virtue of C&P. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java <https://reviews.apache.org/r/7412/#comment25910> Sincerely, your friend //TODO 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/7412/#comment25911> Maybe we should, but javadoc saying so on the public close method doesnt seem that useful. 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/7412/#comment25913> Doesnt look like anything has been done to prevent TransportExceptions being propagated to this level, so I'm not sure that removal is wise. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/client/destination/AddressBasedDestinationTest.java <https://reviews.apache.org/r/7412/#comment25914> The tests could use some javadoc to describe what they are doing and why. - Robbie Gemmell On Oct. 6, 2012, 4:55 p.m., rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7412/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2012, 4:55 p.m.) > > > Review request for qpid, Robbie Gemmell, Weston Price, and Keith Wall. > > > Description > ------- > > The attached patch does the following, > 1. Node bindings are created when the node is created (was there before). > 2. Node bindings are deleted when the node is deleted (added in this patch). > 3. Link bindings are created when a subscription or producer is created (was > there before) > 4. Link bindings are deleted when a subscription or producer is closed (added > in this patch). > > The code was modified to treat node and link bindings separately. > I have also added sync() methods (disregarding the nowait flag) when creating > a producer or consumer to ensure we report the errors correctly. > (In the existing code, When creating consumers the nowait flag was set to > false, while when creating durable subscribers it was set to true.). > > Please ignore the white space errors. The git based patch I have is free of > them. > I had to create an svn diff as rb doesn't accept the git patch. > > > This addresses bug QPID-3317. > https://issues.apache.org/jira/browse/QPID-3317 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQDestination.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_8.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQTopic.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_8.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/messaging/address/AddressHelper.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/messaging/address/Link.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/messaging/address/Node.java > 1394702 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/client/destination/AddressBasedDestinationTest.java > 1394702 > > Diff: https://reviews.apache.org/r/7412/diff/ > > > Testing > ------- > > Used the existing AddressBasedDestinationTest to verify the new work does not > break any functionality. > Added an explicit test case for verifying link beahvior as well as > customizing the subscription queue. > Added unit tests for AddressHelper. > > > Thanks, > > rajith attapattu > >
