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