> On 2011-10-14 16:17:48, Oleksandr Rudyy wrote: > > 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. > > > > 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. > > > > 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. > > > > 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. > > > > 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. > >
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: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2366/ > ----------------------------------------------------------- > > (Updated 2011-10-12 21:02:31) > > > Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith > Wall. > > > Summary > ------- > > This patch contains the new class structure used for retrieving information > from addressing and creating destinations and managing it's lifecycle. > 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/ > > A basic class diagram for this can be found here [ > https://issues.apache.org/jira/secure/attachment/12498753/class_diagram.png ] > > In summary the goals are, > =========================== > 1. Provide a proper abstraction of Queue and Topic concepts > > 2. Provide an address format based implementation of Queue and Topic > > 3. Hide the implementatio of the life cycle of a destination (create, delete, > createSubscription, deleteSubscription) > > 4. Create a top level AddressBasedDestination class (extending from > AMQDestination), > 4.1 To separate the address based details from AMQQueue, AMQTopic ..etc > 4.2 To bridge btw the new code and the AMQDestination interface > > 4. Improve the code that retrievs data from an address (a.k.a AddressHelper) > > 5. Provide a fix for QPID-3265, QPID-3317, QPID-3271 > > 6. Implement the above with minimum disruption to regular client code. > > > This addresses bug QPID-3401. > https://issues.apache.org/jira/browse/QPID-3401 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AddressBasedDestination.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AddressBasedQueue.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AddressBasedTopic.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/QpidDestination.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/QpidQueue.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/QpidTopic.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/Session.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/SubscriptionSettings.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/AddressException.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/AddressHelper.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/AddressProperty.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/AddressResolver.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/Link.java > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/messaging/address/Node.java > PRE-CREATION > > 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 > > 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 > > 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 > > 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 > > 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 > > 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 > > 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 > > 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 > > 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 > > 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 > > 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 > > 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 > > Diff: https://reviews.apache.org/r/2366/diff > > > Testing > ------- > > Changes are verified using the existing tests in > AddressBasedDestinationTest.java > I am planning to add more coverage and possibly refactor the above class to > cover more cases with less code. > > > Thanks, > > rajith > >
