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