> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > 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).
Re: the unit testing. Abosutely agreed with you. I didn't do a lot in this area
bcos I wanted to first get agreement on the design and direction for this
solution.
This patch was aimed at getting agreement and feedback on the design. Perhaps I
should have mentioned my intention under the testing tab.
It seems both you and Rob and are happy with the direction/design. I will now
focus on adding unit tests and further enhancing the code.
I will work on getting the whitespace issue sorted out.
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/DestinationStringParser.java,
> > line 91
> > <https://reviews.apache.org/r/4658/diff/2/?file=100196#file100196line91>
> >
> > BURLs include support for setting various options much like the Address
> > strings, its not clear to me that this really supports them?
I only added support for the main options that I was familiar with. Again this
patch was aimed at getting some agreement on direction. So didn't go into a lot
of detail.
It would be good if I can get some help here on what other options used on your
side. I believe I have covered the options mentioned in the wiki.
I will have a look again.
However somebody else other than myself needs to review this area again when
the work is completed to ensure that I haven't miss any.
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
> > lines 45-47
> > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line45>
> >
> > Unless there is a particular need, we should use getters to access
> > these rather than having the fields themselves protected.
> >
> > Naming convention violations.
are you refering to the lack of leading underscores ?
The reason I left them as protected was to allow the child classes
(QpidQueue/Topic) to easily refer to them.
However I'm fine with marking them private and having getters.
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
> > lines 117-122
> > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line117>
> >
> > This would be better located neat the top of the file close to what it
> > statically initialises.
Agreed
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
> > line 130
> > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line130>
> >
> > 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.
Noted!
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidQueue.java,
> > line 46
> > <https://reviews.apache.org/r/4658/diff/2/?file=100198#file100198line46>
> >
> > Requires a matching hashCode implementation.
Completely forgot this :(
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidQueue.java,
> > lines 59-66
> > <https://reviews.apache.org/r/4658/diff/2/?file=100198#file100198line59>
> >
> > 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.
> >
> >
This was to catch the declared JMSException. There is a potentail for a NPE,
all though rare.
If somebody creates a Queue with an empty constructor but forgets to set the
destination string then this could cause a NPE.
Perhaps we can catch the exception an log it ? the NPE would be due to a
programming error and the log message would help.
What do you think ?
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryQueue.java,
> > line 28
> > <https://reviews.apache.org/r/4658/diff/2/?file=100199#file100199line28>
> >
> > Naming convention
noted !
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryTopic.java,
> > line 28
> > <https://reviews.apache.org/r/4658/diff/2/?file=100200#file100200line28>
> >
> > Naming convention.
noted!
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryTopic.java,
> > line 39
> > <https://reviews.apache.org/r/4658/diff/2/?file=100200#file100200line39>
> >
> > Remove commented out code?
This is not implemented properly. Left that piece as a reminder. Thx for
pointing it out though.
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTopic.java,
> > line 48
> > <https://reviews.apache.org/r/4658/diff/2/?file=100201#file100201line48>
> >
> > Requires a matching hashCode implementation.
agreed.
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/Address.java,
> > lines 45-46
> > <https://reviews.apache.org/r/4658/diff/2/?file=100203#file100203line45>
> >
> > Naming convention
noted! (I assume it's the leading underscore - all though I personally hate it,
I'm quite happy to stick to the convention)
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java,
> > lines 63-74
> > <https://reviews.apache.org/r/4658/diff/2/?file=100205#file100205line63>
> >
> > Using 'protected' for a reason?
> > Naming convention.
Initially thought about subclassing for a protocol version specific child
class, but have since thought otherwise.
I will mark them private.
> On 2012-04-06 10:30:53, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java,
> > lines 87-97
> > <https://reviews.apache.org/r/4658/diff/2/?file=100206#file100206line87>
> >
> > Using 'protected' for a reason?
same as above.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4658/#review6737
-----------------------------------------------------------
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
>
>
