You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by rajith attapattu <ra...@gmail.com> on 2012/04/05 17:03:30 UTC
Review Request: Destination refactoring
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4658/
-----------------------------------------------------------
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
Re: Review Request: Destination refactoring
Posted by rajith attapattu <ra...@gmail.com>.
> 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
>
>
Re: Review Request: Destination refactoring
Posted by Robbie Gemmell <ro...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request: Destination refactoring
Posted by rajith attapattu <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4658/#review7940
-----------------------------------------------------------
Just an update on the status. I have already made the changes suggested here. In addition I have also made further changes based on more comments received by Rob Godfrey.
The only missing piece (a critical one at that) is the testing. I'm planning to add the unit tests soon.
- rajith
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
>
>
Re: Review Request: Destination refactoring
Posted by rajith attapattu <ra...@gmail.com>.
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > Agree with all of Robbie's comments, and added a few more of my own...
> >
> > The work in general looks good, but a) we really need testing on this as it has caused us so many issues in the past and b) we need to do the work to integrate this into the session/consumer/etc - I think adding this code without deleting the old stuff would not make sense.
Yes I totally agree about deleting the old stuff, as soon as we are happy with the integration.
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java, lines 42-43
> > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line42>
> >
> > I think it would be better to move everything to do with DestSyntax to the parser
I totally agree with you!
There is no reason to really have this code in the QpidDestination class.
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java, line 51
> > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line51>
> >
> > Rather than having this as a member variable you could simply define
> >
> > abstract public DestinationType getType();
> >
> > and provide implementations in the two direct subclasses
Agreed!
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java, lines 79-96
> > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line79>
> >
> > Wouldn't it make more sense to move this code to the DestinationStringParser?
agreed as mentioned above.
> On 2012-04-06 11:06:40, Rob Godfrey 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>
> >
> > Move to the parser class?
agreed as mentioned above.
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java, lines 124-144
> > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line124>
> >
> > move to the parser class, also could this not be simplified to
> >
> > return Enum.valueOf(DestSyntax.class, str);
will do.
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java, lines 146-160
> > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line146>
> >
> > Move to parser
agreed as mentioned above.
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java, lines 162-172
> > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line162>
> >
> > Move to parser
agreed as mentioned above.
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidQueue.java, lines 53-56
> > <https://reviews.apache.org/r/4658/diff/2/?file=100198#file100198line53>
> >
> > should this not be
> >
> > if(obj.getClass() != getClass())
> >
> > otherwise you will (incorrectly) have temporary queues equalling non-temporary queues
Good point !
> On 2012-04-06 11:06:40, Rob Godfrey 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>
> >
> > Visibility/access - member variables should be private with setters/getters of appropriate visibility as necessary. Also can this not be made final?
> >
agreed
> On 2012-04-06 11:06:40, Rob Godfrey 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>
> >
> > And access - should be private with package local getter/setter if necessary
agreed
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryTopic.java, lines 37-40
> > <https://reviews.apache.org/r/4658/diff/2/?file=100200#file100200line37>
> >
> > Should not the delete method actually delete? Why is this commented out?
It certainly should!
I wasn't sure how best to keep track of the subscription queue, hence determing the queue-name to delete.
What if the TempTopic was used by more than one Consumer (the same issue for QpidTopic).
Previously I used to clone the Topic object so each consumer is using a different destination object so as to keep the subscription queue name unique.
I wonder if there is a better solution.
I wanted to take a bit of time to think through this, hence commented it out temporarily.
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTopic.java, lines 55-58
> > <https://reviews.apache.org/r/4658/diff/2/?file=100201#file100201line55>
> >
> > Should be
> >
> > if(obj.getClass() != getClass())
> >
> > to avoid spurious equality on objects of subclasses
Agreed!
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/Address.java, lines 81-84
> > <https://reviews.apache.org/r/4658/diff/2/?file=100203#file100203line81>
> >
> > The Node is a mutable object, thus the address is mutable - is this intended? What are the side effects of someone mutating the node on an existing address... what if the node is shared between multiple addresses?
Good question. It wasn't intented to be mutable or shared between multiple addresses.
The setters were added so I could input the address information more easily than shoving them all through a constructor.
One option is to ensure we mark the Node and Link object as read-only once the Destination Parser object completes the filling of information.
Thereafter if someone calls a setter we can throw an exception.
Is this an agreeable solution ?
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/Address.java, lines 91-94
> > <https://reviews.apache.org/r/4658/diff/2/?file=100203#file100203line91>
> >
> > The Link is a mutable object, thus the address is mutable - is this intended? What are the side effects of someone mutating the link on an existing address... what if the link is shared between multiple addresses?
pls see above.
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java, lines 38-60
> > <https://reviews.apache.org/r/4658/diff/2/?file=100205#file100205line38>
> >
> > Why define the set of Reliabilities and then not make a distinction between known but not implemented Reliabilities, and unknown reliabilities?
> >
> > For instance someone using the reliability mode "at_least_once" would get the error message "the reliability mode 'at_least_once' is not yet supported" whereas it would be better to say "Unknown reliability mode 'at_least_once'" you may also wish to ass the list of supported reliability modes to the message to help people if they've made a typo.
Very good suggestion!
I will do that.
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java, lines 116-119
> > <https://reviews.apache.org/r/4658/diff/2/?file=100205#file100205line116>
> >
> > Returning mutable object - wrap in Collections.unmodifiableMap() ?
Agreed!
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java, lines 121-124
> > <https://reviews.apache.org/r/4658/diff/2/?file=100205#file100205line121>
> >
> > Returning mutable object - wrap in Collections.unmodifiableList() ?
Agreed!
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java, lines 126-129
> > <https://reviews.apache.org/r/4658/diff/2/?file=100205#file100205line126>
> >
> > Returning mutable object - wrap in Collections.unmodifiableMap() ?
Agreed!
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java, lines 35-59
> > <https://reviews.apache.org/r/4658/diff/2/?file=100206#file100206line35>
> >
> > Again can one not could simplify this by using the Enum.valueOf(..) method?
> >
> > try
> > {
> > return policy == null ? NEVER : Enum.valueOf(AddressPolicy.class, policy.toUpperCase());
> > }
> > catch (IllegalArgumentException e)
> > {
> > throw new AddressException("Invalid address policy type: '" + policy + "'");
> > }
> >
> > Again, adding the valid values to the error message would be user friendly.
Agreed!
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java, lines 66-84
> > <https://reviews.apache.org/r/4658/diff/2/?file=100206#file100206line66>
> >
> > Use Enum.valueOf(...) ?
Agreed!
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java, lines 129-131
> > <https://reviews.apache.org/r/4658/diff/2/?file=100206#file100206line129>
> >
> > Should we be returning the mutable map here? What happens if the caller mutates the map? Should we not wrap in Collections.unmodifiableMap()?
Agreed!
> On 2012-04-06 11:06:40, Rob Godfrey wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java, lines 134-137
> > <https://reviews.apache.org/r/4658/diff/2/?file=100206#file100206line134>
> >
> > returning mutable List? Wrap in Collections.unmodifiableList() ?
> >
Agreed!
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4658/#review6738
-----------------------------------------------------------
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
>
>
Re: Review Request: Destination refactoring
Posted by Rob Godfrey <rg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4658/#review6738
-----------------------------------------------------------
Agree with all of Robbie's comments, and added a few more of my own...
The work in general looks good, but a) we really need testing on this as it has caused us so many issues in the past and b) we need to do the work to integrate this into the session/consumer/etc - I think adding this code without deleting the old stuff would not make 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/#comment14708>
I think it would be better to move everything to do with DestSyntax to the parser
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/#comment14713>
Rather than having this as a member variable you could simply define
abstract public DestinationType getType();
and provide implementations in the two direct subclasses
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/#comment14707>
Wouldn't it make more sense to move this code to the DestinationStringParser?
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/#comment14709>
Move to the parser class?
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/#comment14710>
move to the parser class, also could this not be simplified to
return Enum.valueOf(DestSyntax.class, str);
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/#comment14711>
Move to parser
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/#comment14712>
Move to parser
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/#comment14706>
should this not be
if(obj.getClass() != getClass())
otherwise you will (incorrectly) have temporary queues equalling non-temporary queues
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/#comment14705>
Visibility/access - member variables should be private with setters/getters of appropriate visibility as necessary. Also can this not be made final?
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/#comment14703>
And access - should be private with package local getter/setter if necessary
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/#comment14704>
Should not the delete method actually delete? Why is this commented out?
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/#comment14714>
Should be
if(obj.getClass() != getClass())
to avoid spurious equality on objects of subclasses
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/#comment14723>
The Node is a mutable object, thus the address is mutable - is this intended? What are the side effects of someone mutating the node on an existing address... what if the node is shared between multiple addresses?
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/#comment14724>
The Link is a mutable object, thus the address is mutable - is this intended? What are the side effects of someone mutating the link on an existing address... what if the link is shared between multiple addresses?
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/#comment14715>
Why define the set of Reliabilities and then not make a distinction between known but not implemented Reliabilities, and unknown reliabilities?
For instance someone using the reliability mode "at_least_once" would get the error message "the reliability mode 'at_least_once' is not yet supported" whereas it would be better to say "Unknown reliability mode 'at_least_once'" you may also wish to ass the list of supported reliability modes to the message to help people if they've made a typo.
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/#comment14720>
Returning mutable object - wrap in Collections.unmodifiableMap() ?
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/#comment14722>
Returning mutable object - wrap in Collections.unmodifiableList() ?
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/#comment14721>
Returning mutable object - wrap in Collections.unmodifiableMap() ?
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/#comment14716>
Again can one not could simplify this by using the Enum.valueOf(..) method?
try
{
return policy == null ? NEVER : Enum.valueOf(AddressPolicy.class, policy.toUpperCase());
}
catch (IllegalArgumentException e)
{
throw new AddressException("Invalid address policy type: '" + policy + "'");
}
Again, adding the valid values to the error message would be user friendly.
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/#comment14717>
Use Enum.valueOf(...) ?
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/#comment14718>
Should we be returning the mutable map here? What happens if the caller mutates the map? Should we not wrap in Collections.unmodifiableMap()?
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/#comment14719>
returning mutable List? Wrap in Collections.unmodifiableList() ?
- Rob
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
>
>