> On 2011-05-10 09:25:19, Gordon Sim wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQAnyDestination.java, > > line 87 > > <https://reviews.apache.org/r/706/diff/1/?file=18457#file18457line87> > > > > Not a major issue, but this seems a bit convoluted to me as compared > > with the more direct: > > > > if (getRoutingKey() != null) { > > return getRoutingKey().toString(); > > } else if (getSubject() != null) { > > return getSubject(); > > } else { > > return ""; > > } > > > > Also why use 'super' here? > > > > Finally, is returning the empty string the right default? Might it be > > better to simply throw an exception? What does it mean if the topic has a > > null routing key (and null subject)?
Agreed about the simpler form and that using supper is unnecessary. Let me start with some background info before I explain the reasons. The AMQAnyDestination is actually a hybrid class that represents both Topics and Queues. A bad idea (by me) in hindsight, that pre dates the introduction of addressing. At the time in our JMS implementation, Topics reffered to amq.topic (or any other Topic exchange) and Queues referred to amq.direct (or any other Direct exchange). The design at that time was a Destination type for each exchange type. I conceived an idea of representing any exchange type with a single destination implementation, and the result was AMQAnyDestination. With the binding URL's and existing design model this worked well. However with the introduction of addressing and it's definition of Topics and Queues (which was more aligned with the JMS notion of Queues and Topics) this quickly became a bad idea. I should have dropped this class in favour of something like QpidQueue and QpidTopic which would have made a lot more sense. However I struggled a bit with merging the existing behaviour with the new behaviour and instead opted for keeping this class. When addressing is used a null routing key (or subject) means that a subject isn't specified in the address string. For queues this should simply mean "" (empty string) and for topics it's the same except when the underlying exchange type was a Topic exchange. In which case the subject was defaulted to "#". The address resolution code is responsible for figuring this out and updates the fields (both subject and routing key) with the right defaults. But until it does so, this fields aren't populated. When BURL's are used the routing key could be null when using the fanout exchange. So clearly we can't throw an exception, so the safe approach is to use "". Also the routing key is of type AMQShortString and in the code there could be instances of just calling getRoutingKey().asString() without checking for null which will result in an NPE. So IMO it's best to use "" as the default until it's updated properly. > On 2011-05-10 09:25:19, Gordon Sim wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, > > line 1060 > > <https://reviews.apache.org/r/706/diff/1/?file=18458#file18458line1060> > > > > What happens here if the address doesn't resolve to an existing > > exchange? If the address doesn't resolve to an existing exchange or queue an exception will be thrown if create option is not selected. But I guess your real question is do we throw an exception if the address resolves to a queue instead of an exchange. At the moment we don't, so we do allow a durable subscription to be created on a Queue. Not sure if this is JMS compliant though. What are your thoughts on this? > On 2011-05-10 09:25:19, Gordon Sim wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, > > line 1081 > > <https://reviews.apache.org/r/706/diff/1/?file=18458#file18458line1081> > > > > As above, is it possible the routing key is null? If so does that > > matter here? If the address is resolved it can't be null as a default would have been assigned. If BURL is used, then we'd be getting a AMQTopic object which should not have a null routing key. (I actually need to verify that to be sure.) - rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/706/#review654 ----------------------------------------------------------- On 2011-05-10 03:38:41, rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/706/ > ----------------------------------------------------------- > > (Updated 2011-05-10 03:38:41) > > > Review request for qpid. > > > Summary > ------- > > The attached patch adds code to resolve the address to determine the correct > defualt for the subject field and also to populate the legacy fields. > Also added null checks in both AMQAnyDestination and AMQTopic to prevent any > NPE. > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQAnyDestination.java > 1099057 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java > 1099288 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQTopic.java > 1099057 > > Diff: https://reviews.apache.org/r/706/diff > > > Testing > ------- > > > Thanks, > > rajith > >
