[
https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13271871#comment-13271871
]
[email protected] commented on QPID-3401:
-----------------------------------------------------
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. > Agree with all of Robbie's comments, and added a few more of my own...
bq. >
bq. > 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.
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
lines 42-43
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line42>
bq. >
bq. > 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.
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
line 51
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line51>
bq. >
bq. > Rather than having this as a member variable you could simply define
bq. >
bq. > abstract public DestinationType getType();
bq. >
bq. > and provide implementations in the two direct subclasses
Agreed!
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
lines 79-96
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line79>
bq. >
bq. > Wouldn't it make more sense to move this code to the
DestinationStringParser?
agreed as mentioned above.
bq. On 2012-04-06 11:06:40, Rob Godfrey 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. > Move to the parser class?
agreed as mentioned above.
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
lines 124-144
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line124>
bq. >
bq. > move to the parser class, also could this not be simplified to
bq. >
bq. > return Enum.valueOf(DestSyntax.class, str);
will do.
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
lines 146-160
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line146>
bq. >
bq. > Move to parser
agreed as mentioned above.
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java,
lines 162-172
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100197#file100197line162>
bq. >
bq. > Move to parser
agreed as mentioned above.
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidQueue.java,
lines 53-56
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100198#file100198line53>
bq. >
bq. > should this not be
bq. >
bq. > if(obj.getClass() != getClass())
bq. >
bq. > otherwise you will (incorrectly) have temporary queues equalling
non-temporary queues
Good point !
bq. On 2012-04-06 11:06:40, Rob Godfrey 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. > Visibility/access - member variables should be private with
setters/getters of appropriate visibility as necessary. Also can this not be
made final?
bq. >
agreed
bq. On 2012-04-06 11:06:40, Rob Godfrey 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. > And access - should be private with package local getter/setter if
necessary
agreed
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryTopic.java,
lines 37-40
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100200#file100200line37>
bq. >
bq. > 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.
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTopic.java,
lines 55-58
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100201#file100201line55>
bq. >
bq. > Should be
bq. >
bq. > if(obj.getClass() != getClass())
bq. >
bq. > to avoid spurious equality on objects of subclasses
Agreed!
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/Address.java,
lines 81-84
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100203#file100203line81>
bq. >
bq. > 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 ?
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/Address.java,
lines 91-94
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100203#file100203line91>
bq. >
bq. > 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.
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java,
lines 38-60
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100205#file100205line38>
bq. >
bq. > Why define the set of Reliabilities and then not make a distinction
between known but not implemented Reliabilities, and unknown reliabilities?
bq. >
bq. > 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.
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java,
lines 116-119
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100205#file100205line116>
bq. >
bq. > Returning mutable object - wrap in Collections.unmodifiableMap() ?
Agreed!
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java,
lines 121-124
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100205#file100205line121>
bq. >
bq. > Returning mutable object - wrap in Collections.unmodifiableList() ?
Agreed!
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java,
lines 126-129
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100205#file100205line126>
bq. >
bq. > Returning mutable object - wrap in Collections.unmodifiableMap() ?
Agreed!
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java,
lines 35-59
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100206#file100206line35>
bq. >
bq. > Again can one not could simplify this by using the Enum.valueOf(..)
method?
bq. >
bq. > try
bq. > {
bq. > return policy == null ? NEVER :
Enum.valueOf(AddressPolicy.class, policy.toUpperCase());
bq. > }
bq. > catch (IllegalArgumentException e)
bq. > {
bq. > throw new AddressException("Invalid address policy type: '" +
policy + "'");
bq. > }
bq. >
bq. > Again, adding the valid values to the error message would be user
friendly.
Agreed!
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java,
lines 66-84
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100206#file100206line66>
bq. >
bq. > Use Enum.valueOf(...) ?
Agreed!
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java,
lines 129-131
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100206#file100206line129>
bq. >
bq. > Should we be returning the mutable map here? What happens if the
caller mutates the map? Should we not wrap in Collections.unmodifiableMap()?
Agreed!
bq. On 2012-04-06 11:06:40, Rob Godfrey wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java,
lines 134-137
bq. > <https://reviews.apache.org/r/4658/diff/2/?file=100206#file100206line134>
bq. >
bq. > returning mutable List? Wrap in Collections.unmodifiableList() ?
bq. >
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:
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: [email protected]
For additional commands, e-mail: [email protected]
