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/05/10 00:09:00 UTC
Re: Review Request: Destination refactoring
> 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
>
>