qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "rajith attapattu" <rajit...@gmail.com>
Subject Re: Review Request: QPID-3254 - NPE when creating a durable subscriber using an addressing string configured in jndi.properties
Date Tue, 10 May 2011 17:29:51 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message