> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote: > > 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? > > rajith attapattu wrote: > Thanks a lot for the comments. I forgot to add the unit test file. > Let me first reply to your comments and add the file later.
As the test doesn't seem to have been added here, I'll review it by looking at the commit. The binding verification performed is a bit lax, it only really confirms that there are 4 bindings for the right queue to exchanges with "amq." at the start of their name. Although it seems unlikely, its still entirely possible that the bindings are actually largely incorrect (all to the same exchange, all to the exchange actually called "amq.", etc etc) and the tests would completely fail to detect it. We should verify in full that all of the expected exchange names are present and have the expected binding keys/arguments. > On Oct. 7, 2012, 7:06 p.m., 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 1362-1365 > > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1362> > > > > Do we need to add it if it is false? > > rajith attapattu wrote: > If the address doesn't specifiy no-local then we will default to the > no-local value specified when creating the consumer. > The noLocal value here is a parameter passed all the way from > AMQSession.java Sure, but what I'm getting at is does the broker do anything with the no-local=false argument other than basically look at it then ignore it? > On Oct. 7, 2012, 7:06 p.m., 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 1493-1496 > > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1493> > > > > Should we add it if its false? > > rajith attapattu wrote: > If the address doesn't specifiy no-local then we will default to the > no-local value specified when creating the consumer. > The noLocal value here is a parameter passed all the way from > AMQSession.java As above comment. > On Oct. 7, 2012, 7:06 p.m., 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 1531 > > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1531> > > > > 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) > > rajith attapattu wrote: > So the bindings (whether specified in node or link properties) are to do > *extra customization* for the respective queue. > > If the type is Queue, then the "queue-name" used in the binding is set to > the 'Queue Name' given in the address (unless an explicit queue name is > specified (possible but doesn't make sense)). > > Now comes the question of what exchange-name to use if an exchange-name > is not given in the address. > The queue is already bound to the namelessExchange (a.k.a > defaultExchange) so there the extra customization in the form of a binding > means they want to bind it to some other exchange. > Besides, the namelessExchange uses the actual "queue-name" for routing. > So there is no point in specifying an extra binding here. > Here the user has specified a key (if not it's an error), therefore he > intends to use another exchange and we assume amq.topic if nothing is > specified. > > This has been existing behavior as well and the C++ client and python > client uses the same if I'm not mistaken. > > I agree with you that the variable name is a bit confusing here. Let me > think of a better name to avoid the confusion. I understand that it is extra customisation and how the default exchange works, I just wanted to verify why amq.topic is assumed and how people are meant to know that it is. It might be worth documenting that behaviour as it isn't mentioned anywhere, thus my question. There are precisely two mentions of amq.topic on the Address Syntax documentation page; the first is incorrect because it is actually talking about exchange types (i.e should just be topic, not amq.topic) and the second is an out of place reference to the example programs on prior pages that explicitly set amq.topic as the address if you fail to provide one. > On Oct. 7, 2012, 7:06 p.m., 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 1534-1542 > > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1534> > > > > We do this in two places (again below), might be worth a method > > extraction if only so they are consistent. > > rajith attapattu wrote: > Agreed. But ignored anyway for the commit? > On Oct. 7, 2012, 7:06 p.m., 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 1350-1358 > > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1350> > > > > If its going to be a durable queue should it have a name called > > TempQueue? What are the ways such a combination could occur? > > rajith attapattu wrote: > The same behavior was there before as well, just that it wasn't obvious > bcos it happened when we called send0_10QueueDeclare. > > To answer your question, according to the address syntax, a link could be > marked as durable, which will result in the subscription queue being declared > durable if it's a Topic. > If somebody does that then they should probably also provide a name via > the 'name' property to avoid the above situation. > I agree that it's not a good combination, but it's possible as it stands. > > Perhaps what we could do is to generate a warning asking them to consider > naming the queue. We should generate a warning, or possibly even throw an exception (though it looks from the commit like you haven't bothered to do either). Generating a durable 'tempqueue' queue you then cant resubscribe to or delete later (if your program crashes, machine restarts, etc etc) is something we should avoid both on grounds its unkind to the users, and it leaves the broker to deal with a durable queue that will likely end up with no consumers. Rather than just saying 'its already broken so lets leave it that way', this seems like something we easily can and thus should fix while rewriting this particular bit of code. > On Oct. 7, 2012, 7:06 p.m., 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 268-287 > > <https://reviews.apache.org/r/7412/diff/1/?file=174501#file174501line268> > > > > Doesnt look like anything has been done to prevent TransportExceptions > > being propagated to this level, so I'm not sure that removal is wise. > > rajith attapattu wrote: > Fair comment. I will add it as another catch clause. > Btw this is why I insit on your reviewing. You rarely miss them :) I'm happy to help review things but looking over your own patch is generally nice before asking others to do so, and I cant help thinking you didn't as that would surely have caught some of the issues like the Thread.dumpStack() and System.out.println() stacktrace output if not also this slightly random exception handling change. - Robbie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7412/#review12215 ----------------------------------------------------------- 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 > >
