Review Board 1.7.22

Destination refactoring

Review Request #4658 - Created April 5, 2012 and updated

rajith attapattu
gordon, rgodfrey, rhs, robbie, wprice
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).

Posted (April 6, 2012, 10:30 a.m.)
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).
  1. 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.
BURLs include support for setting various options much like the Address strings, its not clear to me that this really supports them?
  1. 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.
Naming convention violation.
Would initialising this to the 'defaultDestSyntax' value directly above it not make more sense?
Unless there is a particular need, we should use getters to access these rather than having the fields themselves protected.

Naming convention violations.
  1. 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.
This would be better located neat the top of the file close to what it statically initialises.
  1. Agreed
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.
  1. Noted!
Requires a matching hashCode implementation.
  1. Completely forgot this :(
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.

  1. 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 ?
Naming convention
  1. noted !
Naming convention.
  1. noted!
Remove commented out code?
  1. This is not implemented properly. Left that piece as a reminder. Thx for pointing it out though.
Requires a matching hashCode implementation.
  1. agreed.
Naming convention
  1. noted! (I assume it's the leading underscore - all though I personally hate it, I'm quite happy to stick to the convention)
Using 'protected' for a reason?
Naming convention.
  1. Initially thought about subclassing for a protocol version specific child class, but have since thought otherwise.
    I will mark them private.
Using 'protected' for a reason?
  1. same as above.
Posted (April 6, 2012, 11:06 a.m.)
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.
  1. Yes I totally agree about deleting the old stuff, as soon as we are happy with the integration.
I think it would be better to move everything to do with DestSyntax to the parser
  1. I totally agree with you!
    There is no reason to really have this code in the QpidDestination class.
Rather than having this as a member variable you could simply define

abstract public DestinationType getType();

and provide implementations in the two direct subclasses
  1. Agreed! (Diff revision 2)
Wouldn't it make more sense to move this code to the DestinationStringParser? 
  1. agreed as mentioned above.
Move to the parser class?
  1. agreed as mentioned above. (Diff revision 2)
move to the parser class, also could this not be simplified to 

return Enum.valueOf(DestSyntax.class, str);
  1. will do.
Move to parser
  1. agreed as mentioned above.
Move to parser
  1. agreed as mentioned above.
should this not be 

if(obj.getClass() != getClass())

otherwise you will (incorrectly) have temporary queues equalling non-temporary queues
  1. Good point !
Visibility/access - member variables should be private with setters/getters of appropriate visibility as necessary.  Also can this not be made final?
  1. agreed
And access - should be private with package local getter/setter if necessary
  1. agreed
Should not the delete method actually delete? Why is this commented out?
  1. 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.
Should be 

if(obj.getClass() != getClass())

to avoid spurious equality on objects of subclasses
  1. Agreed!
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?
  1. 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 ?
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?
  1. pls see above. (Diff revision 2)
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. 
  1. Very good suggestion!
    I will do that.
Returning mutable object - wrap in Collections.unmodifiableMap() ?
  1. Agreed!
Returning mutable object - wrap in Collections.unmodifiableList() ?
  1. Agreed!
Returning mutable object - wrap in Collections.unmodifiableMap() ?
  1. Agreed! (Diff revision 2)
Again can one not could simplify this by using the Enum.valueOf(..) method?

    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.
  1. Agreed! (Diff revision 2)
Use Enum.valueOf(...) ?
  1. Agreed!
Should we be returning the mutable map here? What happens if the caller mutates the map? Should we not wrap in Collections.unmodifiableMap()?
  1. Agreed!
returning mutable List? Wrap in Collections.unmodifiableList() ?
  1. Agreed!
Posted (May 16, 2012, 7:40 p.m.)
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.