qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (QPID-3401) Refactor address resolution code
Date Mon, 09 Apr 2012 17:39:18 GMT

    [ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249992#comment-13249992
] 

jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------



bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > 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.
bq.  > 
bq.  > 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.


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/DestinationStringParser.java,
line 91
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100196#file100196line91>
bq.  >
bq.  >     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.


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
lines 45-47
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line45>
bq.  >
bq.  >     Unless there is a particular need, we should use getters to access these rather
than having the fields themselves protected.
bq.  >     
bq.  >     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.


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
lines 117-122
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line117>
bq.  >
bq.  >     This would be better located neat the top of the file close to what it statically
initialises.

Agreed


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
line 130
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line130>
bq.  >
bq.  >     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!


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidQueue.java,
line 46
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100198#file100198line46>
bq.  >
bq.  >     Requires a matching hashCode implementation.

Completely forgot this :(


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidQueue.java,
lines 59-66
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100198#file100198line59>
bq.  >
bq.  >     Should we really be catching all exceptions, or just the ones we expect to occur?
Is this something we would want to log?
bq.  >     
bq.  >     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.
bq.  >     
bq.  >

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 ?


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryQueue.java,
line 28
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100199#file100199line28>
bq.  >
bq.  >     Naming convention

noted !


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryTopic.java,
line 28
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100200#file100200line28>
bq.  >
bq.  >     Naming convention.

noted!


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryTopic.java,
line 39
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100200#file100200line39>
bq.  >
bq.  >     Remove commented out code?

This is not implemented properly. Left that piece as a reminder. Thx for pointing it out though.


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTopic.java,
line 48
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100201#file100201line48>
bq.  >
bq.  >     Requires a matching hashCode implementation.

agreed.


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/Address.java,
lines 45-46
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100203#file100203line45>
bq.  >
bq.  >     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)


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java,
lines 63-74
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100205#file100205line63>
bq.  >
bq.  >     Using 'protected' for a reason?
bq.  >     Naming convention.

Initially thought about subclassing for a protocol version specific child class, but have
since thought otherwise.
I will mark them private.


bq.  On 2012-04-06 10:30:53, Robbie Gemmell wrote:
bq.  > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java,
lines 87-97
bq.  > <https://reviews.apache.org/r/4658/diff/2/?file=100206#file100206line87>
bq.  >
bq.  >     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:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/4658/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2012-04-05 15:03:30)
bq.  
bq.  
bq.  Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, Rafael Schloming,
and Rob Godfrey.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  The following patch is based on the various discussions we've had on the dev list about
restructing our destination implementation.
bq.  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.
bq.  
bq.  A summary of the desgin is as follows,
bq.  
bq.  1. Once initialized with the destination string, the destination objects are immutable.
bq.  2. There are 2 concrete implementations in the form of QpidTopic and QpidQueue.
bq.  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.
bq.  4. Both BURL and Address strings are parsed and adapted into a common data structure.
Internally the code will work with this data structure.
bq.  5. The Destination impl does not depend on any other classes, thus allowing it be used
with the current code or the new client.
bq.  
bq.  (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).
bq.  
bq.  
bq.  This addresses bug QPID-3401.
bq.      https://issues.apache.org/jira/browse/QPID-3401
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/DestinationStringParser.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidQueue.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryQueue.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryTopic.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTopic.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/TemporaryDestinationProvider.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/Address.java
1309769 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/AddressException.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/util/AddressHelper.java
PRE-CREATION 
bq.  
bq.  Diff: https://reviews.apache.org/r/4658/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  rajith
bq.  
bq.


                
> Refactor address resolution code
> --------------------------------
>
>                 Key: QPID-3401
>                 URL: https://issues.apache.org/jira/browse/QPID-3401
>             Project: Qpid
>          Issue Type: Improvement
>          Components: Java Client
>            Reporter: Rajith Attapattu
>            Assignee: Rajith Attapattu
>              Labels: addressing
>             Fix For: Future
>
>         Attachments: QPID-3401-systests.patch, QPID-3401.patch, class_diagram.png, model2.gif
>
>
> After some thought it seems that the following JIRA's would benefit from some reworking
of the address resolution code as the original design had a few flaws based on incorrect understanding
of the address syntax.
> QPID-3265 	
> QPID-3317
> QPID-3271
> The redesign would be minimal and not very disruptive. The goal is to fix certain design
flaws in the current code, rather than a complete redesign. I am planning to reuse as much
code as possible to ensure we don't throw away tested code.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org


Mime
View raw message