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