qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robbie Gemmell" <robbie.gemm...@gmail.com>
Subject Re: Review Request: Destination refactoring
Date Fri, 06 Apr 2012 10:30:53 GMT

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


This looks like a good start, but I am rather concerned at the continued complete lack of
any unit tests for the destination code. Whilst a lot of the clients destination handling
issues have been related to the 'if BURL else' control flows, there have still been numerous
cases where it was things like the Address string parser ("false" vs "False" for example)
or the behaviour of a particular part of the Addressing Syntax implementation that caused
the issues. This stuff is one of the most isolated and easily unit testable segments of code
the client will ever have, and it really needs to be thoroughly unit tested to help ensure
its correctness and more so to aid its maintainability going forward.

This may be related to your mention of whitespace issues you couldn't get rid of but some
of the files are absolutely riddled with leading tabs, which I thought ReviewBoard highlighted
but it doesn't seem to be doing. "CTRL+A CTRL+I" is your friend when using Eclipse (if you
have the couple of use-spaces-for-tabs type options set correctly).


http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/DestinationStringParser.java
<https://reviews.apache.org/r/4658/#comment14702>

    BURLs include support for setting various options much like the Address strings, its not
clear to me that this really supports them?



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java
<https://reviews.apache.org/r/4658/#comment14687>

    Naming convention violation.



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java
<https://reviews.apache.org/r/4658/#comment14689>

    Would initialising this to the 'defaultDestSyntax' value directly above it not make more
sense?



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java
<https://reviews.apache.org/r/4658/#comment14688>

    Unless there is a particular need, we should use getters to access these rather than having
the fields themselves protected.
    
    Naming convention violations.



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java
<https://reviews.apache.org/r/4658/#comment14690>

    This would be better located neat the top of the file close to what it statically initialises.



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java
<https://reviews.apache.org/r/4658/#comment14691>

    Its possibly time we had a constant somewhere for the BURL: and ADDR: prefixes rather
than using literals everywhere. It would make their usage easier to navigate if nothing else.



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidQueue.java
<https://reviews.apache.org/r/4658/#comment14695>

    Requires a matching hashCode implementation.



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidQueue.java
<https://reviews.apache.org/r/4658/#comment14701>

    Should we really be catching all exceptions, or just the ones we expect to occur? Is this
something we would want to log?
    
    If this is to handle the declared JMSException on the getQueueName() implementation then
perhaps we could just remove that, because the implementation doesn't look like it can actually
throw a JMSException.
    
    



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryQueue.java
<https://reviews.apache.org/r/4658/#comment14692>

    Naming convention



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryTopic.java
<https://reviews.apache.org/r/4658/#comment14696>

    Naming convention.



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryTopic.java
<https://reviews.apache.org/r/4658/#comment14693>

    Remove commented out code?



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTopic.java
<https://reviews.apache.org/r/4658/#comment14694>

    Requires a matching hashCode implementation.



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/Address.java
<https://reviews.apache.org/r/4658/#comment14697>

    Naming convention



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java
<https://reviews.apache.org/r/4658/#comment14700>

    Using 'protected' for a reason?
    Naming convention.



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java
<https://reviews.apache.org/r/4658/#comment14699>

    Using 'protected' for a reason?


- Robbie


On 2012-04-05 15:03:30, rajith attapattu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4658/
> -----------------------------------------------------------
> 
> (Updated 2012-04-05 15:03:30)
> 
> 
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, Rafael Schloming,
and Rob Godfrey.
> 
> 
> Summary
> -------
> 
> The following patch is based on the various discussions we've had on the dev list about
restructing our destination implementation.
> As the first phase this patch only includes the new class heirachy. For the second phase
we will tackle the integration into the code base.
> 
> A summary of the desgin is as follows,
> 
> 1. Once initialized with the destination string, the destination objects are immutable.
> 2. There are 2 concrete implementations in the form of QpidTopic and QpidQueue.
> 3. Destinations will be desginated as "Queues" and "Topics" by the users in the jndi
file. This prevents us from having to automagically decide the type.
> 4. Both BURL and Address strings are parsed and adapted into a common data structure.
Internally the code will work with this data structure.
> 5. The Destination impl does not depend on any other classes, thus allowing it be used
with the current code or the new client.
> 
> (There are 2 oe 3 white space errors that I can't seem to get rid of. It seems they are
comming from the diff process).
> 
> 
> This addresses bug QPID-3401.
>     https://issues.apache.org/jira/browse/QPID-3401
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/DestinationStringParser.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidQueue.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryQueue.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryTopic.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTopic.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/TemporaryDestinationProvider.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/Address.java
1309769 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/AddressException.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/util/AddressHelper.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/4658/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> rajith
> 
>


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