[
https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13248234#comment-13248234
]
[email protected] commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
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:
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]
