qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gordon Sim" <g...@redhat.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 09:25:19 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/706/#review654
-----------------------------------------------------------



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQAnyDestination.java
<https://reviews.apache.org/r/706/#comment1312>

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



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/706/#comment1315>

    What happens here if the address doesn't resolve to an existing exchange?



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/706/#comment1313>

    As above, is it possible the routing key is null? If so does that matter here?



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQTopic.java
<https://reviews.apache.org/r/706/#comment1314>

    Same comments about what seems to be to be convoluted logic and possibly a default where
an exception would be clearer...


- Gordon


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