[
https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127723#comment-13127723
]
jirapos...@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
bq. On 2011-10-14 16:17:48, Oleksandr Rudyy wrote:
bq. > There are a number of operations performed based on the Destination
during client execution, e.g. during Consumer creation and closing, Producer
creation and closing amongst others. Each of these might be implemented
differently depending on the destination syntax and settings in use, and then
further differences can emerge based on any protocol-specific requirements.
Currently, everywhere differences relating to the syntax in use occur we
generally have an if-else statement. This is not a maintainable implementation
approach, and needs to be abstracted away by a common Destination model that
allows grouping of syntax specific operations into separate classes rather than
spreading decisions throughout the code.
bq. >
bq. > The design alterations here improve on some of the deficiencies of the
previous implementation (e.g. the policy validation no longer being within the
main session etc implementation code), but in other ways seems worse. For
example, it appears to result in us having Destination implementations that are
both syntax AND protocol specific, and which have, and require, intimate
knowledge of the Session implementation. This does not seem like progress and
still leaves us with all the if-else statements throughout the client. Also, it
seems that as AddressBasedDestination is not abstract, it will now be possible
for our Destination objects to contain a delegate which implement either the
JMS Topic or Queue interfaces, but not implement it themselves.
bq. >
bq. > In order to eliminate the if-else statements for behaviour which varies
depending on the destination syntax, we can isolate that code in specific
delegate classes for use with Destination objects. Such delegates would
implement a shared interface that declares the varying address related
operations, and could exist for both BindingURL and Address based Destinations.
The different behaviours could be contained within the destination delegates,
however specific implementations of operations such as queue creation, queue
deletion, subscription creation, would be would found within the
protocol-specific session implementations that actually perform these
operations.
bq. >
bq. > The class diagram at
https://issues.apache.org/jira/secure/attachment/12499057/model2.gif depicts
what some of such a Destination model would look like. AMQDestination would be
refactored such that BindingURL specific functionality and Address specific
functionality was moved to subclasses, with all 3 being abstract, and these
used as the basis for implementations of the Queue and Topic interfaces
specific to each URL syntax.
bq. >
bq. > Additionally, if would be nice to have immutable destination object in
order to resolve current issues with thread safety which in turn will allow
storing and reusing them via JNDI.
bq. >
Alex,
As you correctly pointed out the class diagram is infact what I intend as a
Destination model. However given the time constraints and to limit the impact
on the code I had to go for a stop gap measure. The AddressBasedDestination was
infact a bridge btw the new code and the existing code to minimize impact and
to keep things simple for the time being.
The end goal is for the main code to use the QpidDestinaiton abstraction rather
than the horrible if-else stuff. But getting there is a multi-step process with
a serious of patches and is definitely out of scope for a single patch.
<quote> For example, it appears to result in us having Destination
implementations that are both syntax AND protocol specific, and which have, and
require, intimate knowledge of the Session implementation </quote>
First of all from a protocol stand point AMQP it self has realized that things
like createQueue/Exchange ..etc should not be part of the core protocol and as
session methods. From AMQP 1.0 these methods are no longer present in the
session!
As mentioned in the review, the Destinations I envisioned are smart objects
which hides behind an interface (QpidDestination and QpidQueue QpidTopic where
applicable),
(1) does not **leak** information about how it's constructed (i.e BURL syntax
or the address syntax)
(2) does not require any objects using QpidDestination to have any knowledge
about the specific syntax.
(3) does not require any objects using QpidDestination to know how to
create/delete/assert create/delete subscriptions
The c++ client has adopted a similar strategy and seems far more clear and
compact than our current strategy.
If you delegate the above operations to sessions, then not only they require
details about how they are created, they also require details about the
behaviour of a specific syntax.
But if you hide them behind Destination implementations, then the Sessions can
be easily used with any Destination implementation without code changes.
That is a main goal of this refactor.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2366/#review2589
-----------------------------------------------------------
On 2011-10-12 21:02:31, rajith attapattu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2366/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-12 21:02:31)
bq.
bq.
bq. Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and
Keith Wall.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This patch contains the new class structure used for retrieving
information from addressing and creating destinations and managing it's
lifecycle.
bq. How this code is tied to the main client is illustrated with the patch put
up for review at https://reviews.apache.org/r/2364/
bq.
bq. A basic class diagram for this can be found here [
https://issues.apache.org/jira/secure/attachment/12498753/class_diagram.png ]
bq.
bq. In summary the goals are,
bq. ===========================
bq. 1. Provide a proper abstraction of Queue and Topic concepts
bq.
bq. 2. Provide an address format based implementation of Queue and Topic
bq.
bq. 3. Hide the implementatio of the life cycle of a destination (create,
delete, createSubscription, deleteSubscription)
bq.
bq. 4. Create a top level AddressBasedDestination class (extending from
AMQDestination),
bq. 4.1 To separate the address based details from AMQQueue, AMQTopic ..etc
bq. 4.2 To bridge btw the new code and the AMQDestination interface
bq.
bq. 4. Improve the code that retrievs data from an address (a.k.a
AddressHelper)
bq.
bq. 5. Provide a fix for QPID-3265, QPID-3317, QPID-3271
bq.
bq. 6. Implement the above with minimum disruption to regular client code.
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/client/AddressBasedDestination.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AddressBasedQueue.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AddressBasedTopic.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/QpidDestination.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/QpidQueue.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/QpidTopic.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/Session.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/SubscriptionSettings.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/AddressException.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/AddressHelper.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/AddressProperty.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/AddressResolver.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/Link.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/Node.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/amqp_0_10/AddressHelper_0_10.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/amqp_0_10/AddressProperty_0_10.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/amqp_0_10/AddressResolver_0_10.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/amqp_0_10/Binding.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/amqp_0_10/ExchangeNode.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/amqp_0_10/Link_0_10.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/amqp_0_10/Node_0_10.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/amqp_0_10/QpidQueue_0_10.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/amqp_0_10/QpidTopic_0_10.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/amqp_0_10/QueueNode.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/amqp_0_10/SubscriptionSettings_0_10.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/amqp_0_10/Session_0_10.java
PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/2366/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Changes are verified using the existing tests in
AddressBasedDestinationTest.java
bq. I am planning to add more coverage and possibly refactor the above class
to cover more cases with less code.
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
> Fix For: 0.14
>
> 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
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscr...@qpid.apache.org
