You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by "Rajith Attapattu (JIRA)" <ji...@apache.org> on 2011/08/09 17:54:27 UTC
[jira] [Created] (QPID-3401) Refactor address resolution code
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
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.
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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127723#comment-13127723 ]
jiraposter@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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126152#comment-13126152 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2366/
-----------------------------------------------------------
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
> 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: class_diagram.png
>
>
> 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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126154#comment-13126154 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/
-----------------------------------------------------------
(Updated 2011-10-12 21:04:03.513495)
Review request for Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
Summary (updated)
-------
The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
(Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
In summary the changes are,
1. The code now uses AddressBasedDestination if the syntax is ADDR.
2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
3. The code also delegates creating of subscriptions.
TODO.
1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
(AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
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/BasicMessageProducer_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
Diff: https://reviews.apache.org/r/2364/diff
Testing
-------
All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
Thanks,
rajith
> 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: class_diagram.png
>
>
> 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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127661#comment-13127661 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
bq. On 2011-10-14 13:46:19, Robbie Gemmell wrote:
bq. > Alex and I have given the 2 posts a look over. This review represents both our thoughts and contains comments on https://reviews.apache.org/r/2364 and https://reviews.apache.org/r/2366
bq. >
bq. > The lack of proper testing is for us a barrier to these changes being made so close to the release process beginning, whether the code appeared OK or not. The level of change here is fairly significant to be making this close to the release process without proper confidence through testing, however the current testing does not give that confidence and has thus far proven woefully inadequate. Over 18 months since the current Addressing syntax implementation was added, we are still spotting numerous severe issues relating to its use, none of which are caught by the existing tests and so may or may not still exist in this new/refactored implementation. For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
bq. >
bq. > There is a complete lack of unit tests for the Addressing code, both for the current and new implementations. We should be aiming to maximize the amount of unit testing we have, as they are faster to run than system tests, can be much more specific/targeted, and help make it clearer what is and isn't being tested. This must be rectified before commit, not after.
bq. >
bq. > That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after. Does that functionality work in the existing implementation, and are there any tests for it?
bq. >
bq. > Further to the concerns around testing, the next biggest concern would be the new design itself. For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements.
bq. >
bq. > The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist. Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.
bq. >
bq. > For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?
bq. >
bq. > Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.
bq. >
bq. > It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues).
bq.
bq. rajith attapattu wrote:
bq. First up, thanks for taking the time to look at the patches. I appreciate it.
bq. As for the testing situation I agree that the current suite doesn't adequately cover the area in question. This is a topic that I was hoping to tackle after the release. In fact our current test suite doesn't seem to inspire confidence in many situations. As for the comment on unit testing. I will ensure that there is adequate coverage where is make sense. I will put up a patch for review.
bq. I also hear your concern about this being done close to the release and have taken that into account in deciding to hold on to it until the trunk is open again.
bq.
bq. >>For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
bq. Could you please provide more details here? Is there a JIRA regarding this? I will investigate this issue and ensure it's covered in the new implementation.
bq.
bq. >>That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after.
bq. Sorry about the confusion. What I meant was that durable subscriptions will not work with this patch and additional work is required. My intention was to post a follow up patch and as a separate review. I held back as there was already two reviews in progress.
bq. However this patch will only be committed alongside the durable subscription patch.
bq.
bq. >>Does that functionality work in the existing implementation, and are there any tests for it?
bq. It does work with the existing functionality. testDurableSubscriber() in AddressBasedDestinationTest.java
bq. However we should have destination syntax independent tests so we could leverage the existing durable subscriber tests (all though they themselves are a bit thin).
bq. In fact in general our test suite should be able to be independent of destination, sessions, connection implementations etc..
bq.
bq. I will address your comments related to the design in a separate comment.
bq.
Let me discuss your points on the design here. Actually this was one of the goals of putting this up for review. I want to ensure we get our design right.
<quote> For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements. </quote>
I am glad you raised this point. All though not clearly visible with the patch as it doesn't go that far, this is in fact 'the' main goal of the re-factoring.
If you look closely org.apache.qpid.messaging.address.amqp_0_10 contains an <b> 0-10 specific </b> implementation of the <b> address based </b> implementation of a "QpidDestination".
(*) You could also have other syntax based implementations or,
(*) Address based implementations for various protocol versions.
The rest of the code will be working with QpidDestination interface (or QpidTopic & QpidQueue where necessary) without having to have any knowledge about how the destination was constructed (using a particular syntax) or the finer details of how to create/assert/delete (or even sending and receiving) for a specific protocol version for the respective Destination.
Unfortunately given the time constraints and in order to keep the scope relatively small, I didn't go that far (a fact I clearly mentioned in the description).
I intend to do this step by step.
Therefore I ended up bridging the new abstraction with the AMQxxx classes via the AddressBasedDestination class. The various AMQxxx classes makes it quite difficult to get to that state without significant work. So I ended up with the bridging mechanism and the dreaded if(BURL)[else(FOO)] for the time being.
<quote>The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist.</quote>
I quite disagree with this assessment. The main goal here was to provide a clean abstraction to the rest of the code via the QpidDestination interface without them having to worry about syntax and protocol specifics. It's the underlying "protocol session" that contains the "protocol method" to create/delete queues not really your high level Session implementation. In fact from AMQP 1.0 the protocol session no longer has these methods.
If you make the Destinations fairly dumb then you end up complicating your session implementations.
Your session implementation will need to know the specifics for a particular syntax (links, nodes ..etc) and also the protocol specifics in order retrieve information from the destination and invoke the protocol specific methods.
If you want to support multiple syntaxes (our current situation) then you end up with the dreaded if (BURL) situation.
One only needs to look at the AMQSession.java , AMQSession_0_8.java and AMQSession_0_10.java to see why it's a bad idea to have high level session implementations that are protocol specific.
IMO the destinations should be immutable and is smart enough to know what it needs to do and how to do it.
<quote>Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.</quote>
I am not quite sure what you meant here. Could you elaborate more here on how it makes testing difficult? (keeping in mind my above explanation)
<quote>It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues). </quote>
IMO Destinations should be immutable once it's created from a string (or more broadly a specification, where the simplest form being a string) !
If you need to say create a copy, you could do deepCopy() with specific parameters rather than having setters on the Destination.
Creating a durable subscription is a good example here. One reason why I wanted to submit as a separate patch.
I wanted a way to create the new Topic from the given topic with durability, but not by invoking a setter like setDurable on the newly created Topic.
However I couldn't go that far in a single step due to the way our current code works.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2583
-----------------------------------------------------------
On 2011-10-12 21:09:40, rajith attapattu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2364/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-12 21:09:40)
bq.
bq.
bq. Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
bq. (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
bq.
bq. Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
bq.
bq. In summary the changes are,
bq. 1. The code now uses AddressBasedDestination if the syntax is ADDR.
bq. 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
bq. 3. The code also delegates creating of subscriptions.
bq.
bq. TODO.
bq. 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
bq. 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
bq.
bq. (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
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/BasicMessageProducer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
bq.
bq. Diff: https://reviews.apache.org/r/2364/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126671#comment-13126671 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2366/#review2549
-----------------------------------------------------------
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
<https://reviews.apache.org/r/2366/#comment5722>
should be amq.topic
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
<https://reviews.apache.org/r/2366/#comment5723>
should be amq.topic
- Andrew
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
>
>
> 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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126708#comment-13126708 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2366/#review2551
-----------------------------------------------------------
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AddressBasedDestination.java
<https://reviews.apache.org/r/2366/#comment5725>
I assume this and similar mean resolveAddress? Also, is there any particular reason why you can't go ahead and attempt address resolution here? Or is this actually a case of an unresolvABLE address, when destination is null? In the client, resolveAddress, create and azzert would be called sequentially, normally, right?
- Andrew
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
>
>
> 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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127664#comment-13127664 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2366/#review2589
-----------------------------------------------------------
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.
- Oleksandr
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-subscribe@qpid.apache.org
[jira] [Updated] (QPID-3401) Refactor address resolution code
Posted by "Rajith Attapattu (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Rajith Attapattu updated QPID-3401:
-----------------------------------
Attachment: class_diagram.png
Class diagram that describes the new address implementation
> 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: class_diagram.png
>
>
> 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-subscribe@qpid.apache.org
[jira] [Closed] (QPID-3401) Refactor address resolution code
Posted by "Rajith Attapattu (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Rajith Attapattu closed QPID-3401.
----------------------------------
Resolution: Fixed
The work done on address-refactor2 branch will not be retrofitted into the current client. Instead the work will be used in the new client.
Closing this JIRA as this was specifically targeted at the old client.
> 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
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127716#comment-13127716 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
bq. On 2011-10-13 15:34:48, Andrew Kennedy wrote:
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, line 80
bq. > <https://reviews.apache.org/r/2366/diff/1/?file=49795#file49795line80>
bq. >
bq. > should be amq.topic
I actually caught this during testing and fixed it. In order to get a clean patch I applied selectively to a clean branch and in that process may have missed it.
Thanks for picking this up.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2366/#review2549
-----------------------------------------------------------
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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127610#comment-13127610 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2584
-----------------------------------------------------------
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5792>
This new isTopic(Destination) call doesnt seem like an improvement. If a destination is a Topic, then it should implement the Topic interface and the existing check is sufficient.
Looking at the implementation of the isTopic method in AMQDestination, it seems questionable since its perfectly possible for queues bound to amq.topic not to be getting consumed from as JMS Topics.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5793>
Is this condition being used as an equivelent to if(BURL)else(ADDR)?
This looks like it is in need of some abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5794>
Repeated if(AMQTopic)else() conditions, could use abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5795>
Repeated if(AMQTopic)else() conditions, could use abstraction.
Can the existing methods for exchangeName and queueName be overriden in the AddressBasedTopic implementation?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5796>
Is the Subject used as the queueName for AddressBasedTopics? I thought it was used as the routing key?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5797>
Repeated if(AMQTopic)else() conditions, could use abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5798>
Candidate for some sort of Destination factory?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5799>
Candidate for some sort of Destination factory?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5800>
The old way seems better. If I add another Topic implementation later on, I'd now have to track this line down and update it.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5801>
See earlier comment on this.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5802>
This doesnt seem to be used?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5803>
Why does this catch an Exception now when it didnt before? Also, Exception rather than something more specific?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5807>
The ever-maintainable if-else pattern. Abstraction required.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5804>
As per previous comments on the isTopic() changes in AMQSession, this condition seems questionable. If a Topic destination wasnt provided, then overriding what it is classed as based on the exchange in use seems invalid. Also, this looks to clash with logic used in BasicMessageConsumer which decides the value of preAcquire in a different way (which is in itself a bug we were already aiming to fix by consolidating those types of decision into 1 place).
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5806>
The Destination being an instance of Topic would seem to be a more robust check here. We should always make sure that is true for all our Topic destination Objects.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5809>
It seems like at least some of these removed methods were better placed where they were.
It doesnt seem unreasonable for the protocol-specific implementation of operations depending solely on the session to actually be in the protocol-specific Session implementations. Better here than in the Destination as some of the functionality now appears to be.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
<https://reviews.apache.org/r/2364/#comment5810>
Yet more if(ADDR), needs abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
<https://reviews.apache.org/r/2364/#comment5811>
As before, why do we now throw an exception, and why do we need to catch all Exceptions here ?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
<https://reviews.apache.org/r/2364/#comment5812>
All the casting here seems ugly, and this is an operation ultimately performed by the session so it would seem cleaner if its implementation was ultimately part of the session.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5814>
Part of the ever-present if-else pattern, needs abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5813>
Whitespace error introduced (along with a vast number of others) despite no other change around this code.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5815>
Part of the ever-present if-else pattern, needs abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5816>
TODO ?
- Robbie
On 2011-10-12 21:09:40, rajith attapattu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2364/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-12 21:09:40)
bq.
bq.
bq. Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
bq. (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
bq.
bq. Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
bq.
bq. In summary the changes are,
bq. 1. The code now uses AddressBasedDestination if the syntax is ADDR.
bq. 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
bq. 3. The code also delegates creating of subscriptions.
bq.
bq. TODO.
bq. 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
bq. 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
bq.
bq. (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
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/BasicMessageProducer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
bq.
bq. Diff: https://reviews.apache.org/r/2364/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
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
>
>
> 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-subscribe@qpid.apache.org
[jira] [Updated] (QPID-3401) Refactor address resolution code
Posted by "Rajith Attapattu (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Rajith Attapattu updated QPID-3401:
-----------------------------------
Fix Version/s: (was: 0.15)
Future
> 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
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127667#comment-13127667 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
bq. On 2011-10-14 13:46:19, Robbie Gemmell wrote:
bq. > Alex and I have given the 2 posts a look over. This review represents both our thoughts and contains comments on https://reviews.apache.org/r/2364 and https://reviews.apache.org/r/2366
bq. >
bq. > The lack of proper testing is for us a barrier to these changes being made so close to the release process beginning, whether the code appeared OK or not. The level of change here is fairly significant to be making this close to the release process without proper confidence through testing, however the current testing does not give that confidence and has thus far proven woefully inadequate. Over 18 months since the current Addressing syntax implementation was added, we are still spotting numerous severe issues relating to its use, none of which are caught by the existing tests and so may or may not still exist in this new/refactored implementation. For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
bq. >
bq. > There is a complete lack of unit tests for the Addressing code, both for the current and new implementations. We should be aiming to maximize the amount of unit testing we have, as they are faster to run than system tests, can be much more specific/targeted, and help make it clearer what is and isn't being tested. This must be rectified before commit, not after.
bq. >
bq. > That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after. Does that functionality work in the existing implementation, and are there any tests for it?
bq. >
bq. > Further to the concerns around testing, the next biggest concern would be the new design itself. For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements.
bq. >
bq. > The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist. Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.
bq. >
bq. > For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?
bq. >
bq. > Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.
bq. >
bq. > It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues).
bq.
bq. rajith attapattu wrote:
bq. First up, thanks for taking the time to look at the patches. I appreciate it.
bq. As for the testing situation I agree that the current suite doesn't adequately cover the area in question. This is a topic that I was hoping to tackle after the release. In fact our current test suite doesn't seem to inspire confidence in many situations. As for the comment on unit testing. I will ensure that there is adequate coverage where is make sense. I will put up a patch for review.
bq. I also hear your concern about this being done close to the release and have taken that into account in deciding to hold on to it until the trunk is open again.
bq.
bq. >>For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
bq. Could you please provide more details here? Is there a JIRA regarding this? I will investigate this issue and ensure it's covered in the new implementation.
bq.
bq. >>That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after.
bq. Sorry about the confusion. What I meant was that durable subscriptions will not work with this patch and additional work is required. My intention was to post a follow up patch and as a separate review. I held back as there was already two reviews in progress.
bq. However this patch will only be committed alongside the durable subscription patch.
bq.
bq. >>Does that functionality work in the existing implementation, and are there any tests for it?
bq. It does work with the existing functionality. testDurableSubscriber() in AddressBasedDestinationTest.java
bq. However we should have destination syntax independent tests so we could leverage the existing durable subscriber tests (all though they themselves are a bit thin).
bq. In fact in general our test suite should be able to be independent of destination, sessions, connection implementations etc..
bq.
bq. I will address your comments related to the design in a separate comment.
bq.
bq.
bq. rajith attapattu wrote:
bq. Let me discuss your points on the design here. Actually this was one of the goals of putting this up for review. I want to ensure we get our design right.
bq.
bq. <quote> For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements. </quote>
bq.
bq. I am glad you raised this point. All though not clearly visible with the patch as it doesn't go that far, this is in fact 'the' main goal of the re-factoring.
bq.
bq. If you look closely org.apache.qpid.messaging.address.amqp_0_10 contains an <b> 0-10 specific </b> implementation of the <b> address based </b> implementation of a "QpidDestination".
bq.
bq. (*) You could also have other syntax based implementations or,
bq. (*) Address based implementations for various protocol versions.
bq.
bq. The rest of the code will be working with QpidDestination interface (or QpidTopic & QpidQueue where necessary) without having to have any knowledge about how the destination was constructed (using a particular syntax) or the finer details of how to create/assert/delete (or even sending and receiving) for a specific protocol version for the respective Destination.
bq.
bq. Unfortunately given the time constraints and in order to keep the scope relatively small, I didn't go that far (a fact I clearly mentioned in the description).
bq. I intend to do this step by step.
bq.
bq. Therefore I ended up bridging the new abstraction with the AMQxxx classes via the AddressBasedDestination class. The various AMQxxx classes makes it quite difficult to get to that state without significant work. So I ended up with the bridging mechanism and the dreaded if(BURL)[else(FOO)] for the time being.
bq.
bq.
bq. <quote>The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist.</quote>
bq.
bq. I quite disagree with this assessment. The main goal here was to provide a clean abstraction to the rest of the code via the QpidDestination interface without them having to worry about syntax and protocol specifics. It's the underlying "protocol session" that contains the "protocol method" to create/delete queues not really your high level Session implementation. In fact from AMQP 1.0 the protocol session no longer has these methods.
bq.
bq. If you make the Destinations fairly dumb then you end up complicating your session implementations.
bq. Your session implementation will need to know the specifics for a particular syntax (links, nodes ..etc) and also the protocol specifics in order retrieve information from the destination and invoke the protocol specific methods.
bq.
bq. If you want to support multiple syntaxes (our current situation) then you end up with the dreaded if (BURL) situation.
bq. One only needs to look at the AMQSession.java , AMQSession_0_8.java and AMQSession_0_10.java to see why it's a bad idea to have high level session implementations that are protocol specific.
bq.
bq. IMO the destinations should be immutable and is smart enough to know what it needs to do and how to do it.
bq.
bq. <quote>Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.</quote>
bq.
bq. I am not quite sure what you meant here. Could you elaborate more here on how it makes testing difficult? (keeping in mind my above explanation)
bq.
bq.
bq. <quote>It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues). </quote>
bq.
bq. IMO Destinations should be immutable once it's created from a string (or more broadly a specification, where the simplest form being a string) !
bq. If you need to say create a copy, you could do deepCopy() with specific parameters rather than having setters on the Destination.
bq.
bq. Creating a durable subscription is a good example here. One reason why I wanted to submit as a separate patch.
bq. I wanted a way to create the new Topic from the given topic with durability, but not by invoking a setter like setDurable on the newly created Topic.
bq.
bq. However I couldn't go that far in a single step due to the way our current code works.
<quote>Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.</quote>
I agree with you about Destinations being resolved per underlying protocol connection (not the high level JMS destination) as administrative (or other) changes may have made the previous resolution invalid. Infact a Destination should be resolved every time it's being used to create a producer or consumer.
However due to a deadlock issue this was difficult to do in the current code base (see QPID-2808)
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2583
-----------------------------------------------------------
On 2011-10-12 21:09:40, rajith attapattu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2364/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-12 21:09:40)
bq.
bq.
bq. Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
bq. (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
bq.
bq. Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
bq.
bq. In summary the changes are,
bq. 1. The code now uses AddressBasedDestination if the syntax is ADDR.
bq. 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
bq. 3. The code also delegates creating of subscriptions.
bq.
bq. TODO.
bq. 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
bq. 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
bq.
bq. (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
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/BasicMessageProducer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
bq.
bq. Diff: https://reviews.apache.org/r/2364/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247270#comment-13247270 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4658/
-----------------------------------------------------------
Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, Rafael Schloming, and Rob Godfrey.
Summary
-------
The following patch is based on the various discussions we've had on the dev list about restructing our destination implementation.
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.
A summary of the desgin is as follows,
1. Once initialized with the destination string, the destination objects are immutable.
2. There are 2 concrete implementations in the form of QpidTopic and QpidQueue.
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.
4. Both BURL and Address strings are parsed and adapted into a common data structure. Internally the code will work with this data structure.
5. The Destination impl does not depend on any other classes, thus allowing it be used with the current code or the new client.
(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).
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/jms/DestinationStringParser.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidDestination.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidQueue.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryQueue.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTemporaryTopic.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/QpidTopic.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/TemporaryDestinationProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/Address.java 1309769
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/AddressException.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Link.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/address/Node.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/messaging/util/AddressHelper.java PRE-CREATION
Diff: https://reviews.apache.org/r/4658/diff
Testing
-------
Thanks,
rajith
> 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
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126161#comment-13126161 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/
-----------------------------------------------------------
(Updated 2011-10-12 21:09:40.553855)
Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
Summary
-------
The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
(Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
In summary the changes are,
1. The code now uses AddressBasedDestination if the syntax is ADDR.
2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
3. The code also delegates creating of subscriptions.
TODO.
1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
(AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
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/BasicMessageProducer_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
Diff: https://reviews.apache.org/r/2364/diff
Testing
-------
All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
Thanks,
rajith
> 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
>
>
> 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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13248226#comment-13248226 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4658/#review6737
-----------------------------------------------------------
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.
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).
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/DestinationStringParser.java
<https://reviews.apache.org/r/4658/#comment14702>
BURLs include support for setting various options much like the Address strings, its not clear to me that this really supports them?
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/#comment14687>
Naming convention violation.
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/#comment14689>
Would initialising this to the 'defaultDestSyntax' value directly above it not make more 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/#comment14688>
Unless there is a particular need, we should use getters to access these rather than having the fields themselves protected.
Naming convention violations.
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/#comment14690>
This would be better located neat the top of the file close to what it statically initialises.
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/#comment14691>
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.
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/#comment14695>
Requires a matching hashCode implementation.
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/#comment14701>
Should we really be catching all exceptions, or just the ones we expect to occur? Is this something we would want to log?
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.
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/#comment14692>
Naming convention
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/#comment14696>
Naming convention.
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/#comment14693>
Remove commented out code?
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/#comment14694>
Requires a matching hashCode implementation.
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/#comment14697>
Naming convention
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/#comment14700>
Using 'protected' for a reason?
Naming convention.
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/#comment14699>
Using 'protected' for a reason?
- Robbie
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
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13277049#comment-13277049 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4658/#review7940
-----------------------------------------------------------
Just an update on the status. I have already made the changes suggested here. In addition I have also made further changes based on more comments received by Rob Godfrey.
The only missing piece (a critical one at that) is the testing. I'm planning to add the unit tests soon.
- rajith
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
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127553#comment-13127553 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2583
-----------------------------------------------------------
Alex and I have given the 2 posts a look over. This review represents both our thoughts and contains comments on https://reviews.apache.org/r/2364 and https://reviews.apache.org/r/2366
The lack of proper testing is for us a barrier to these changes being made so close to the release process beginning, whether the code appeared OK or not. The level of change here is fairly significant to be making this close to the release process without proper confidence through testing, however the current testing does not give that confidence and has thus far proven woefully inadequate. Over 18 months since the current Addressing syntax implementation was added, we are still spotting numerous severe issues relating to its use, none of which are caught by the existing tests and so may or may not still exist in this new/refactored implementation. For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
There is a complete lack of unit tests for the Addressing code, both for the current and new implementations. We should be aiming to maximize the amount of unit testing we have, as they are faster to run than system tests, can be much more specific/targeted, and help make it clearer what is and isn't being tested. This must be rectified before commit, not after.
That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after. Does that functionality work in the existing implementation, and are there any tests for it?
Further to the concerns around testing, the next biggest concern would be the new design itself. For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements.
The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist. Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.
For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?
Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.
It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues).
- Robbie
On 2011-10-12 21:09:40, rajith attapattu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2364/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-12 21:09:40)
bq.
bq.
bq. Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
bq. (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
bq.
bq. Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
bq.
bq. In summary the changes are,
bq. 1. The code now uses AddressBasedDestination if the syntax is ADDR.
bq. 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
bq. 3. The code also delegates creating of subscriptions.
bq.
bq. TODO.
bq. 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
bq. 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
bq.
bq. (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
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/BasicMessageProducer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
bq.
bq. Diff: https://reviews.apache.org/r/2364/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
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
>
>
> 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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127614#comment-13127614 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
bq. On 2011-10-14 13:46:19, Robbie Gemmell wrote:
bq. > Alex and I have given the 2 posts a look over. This review represents both our thoughts and contains comments on https://reviews.apache.org/r/2364 and https://reviews.apache.org/r/2366
bq. >
bq. > The lack of proper testing is for us a barrier to these changes being made so close to the release process beginning, whether the code appeared OK or not. The level of change here is fairly significant to be making this close to the release process without proper confidence through testing, however the current testing does not give that confidence and has thus far proven woefully inadequate. Over 18 months since the current Addressing syntax implementation was added, we are still spotting numerous severe issues relating to its use, none of which are caught by the existing tests and so may or may not still exist in this new/refactored implementation. For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
bq. >
bq. > There is a complete lack of unit tests for the Addressing code, both for the current and new implementations. We should be aiming to maximize the amount of unit testing we have, as they are faster to run than system tests, can be much more specific/targeted, and help make it clearer what is and isn't being tested. This must be rectified before commit, not after.
bq. >
bq. > That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after. Does that functionality work in the existing implementation, and are there any tests for it?
bq. >
bq. > Further to the concerns around testing, the next biggest concern would be the new design itself. For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements.
bq. >
bq. > The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist. Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.
bq. >
bq. > For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?
bq. >
bq. > Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.
bq. >
bq. > It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues).
First up, thanks for taking the time to look at the patches. I appreciate it.
As for the testing situation I agree that the current suite doesn't adequately cover the area in question. This is a topic that I was hoping to tackle after the release. In fact our current test suite doesn't seem to inspire confidence in many situations. As for the comment on unit testing. I will ensure that there is adequate coverage where is make sense. I will put up a patch for review.
I also hear your concern about this being done close to the release and have taken that into account in deciding to hold on to it until the trunk is open again.
bq. >For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
Could you please provide more details here? Is there a JIRA regarding this? I will investigate this issue and ensure it's covered in the new implementation.
bq. >That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after.
Sorry about the confusion. What I meant was that durable subscriptions will not work with this patch and additional work is required. My intention was to post a follow up patch and as a separate review. I held back as there was already two reviews in progress.
However this patch will only be committed alongside the durable subscription patch.
bq. >Does that functionality work in the existing implementation, and are there any tests for it?
It does work with the existing functionality. testDurableSubscriber() in AddressBasedDestinationTest.java
However we should have destination syntax independent tests so we could leverage the existing durable subscriber tests (all though they themselves are a bit thin).
In fact in general our test suite should be able to be independent of destination, sessions, connection implementations etc..
I will address your comments related to the design in a separate comment.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2583
-----------------------------------------------------------
On 2011-10-12 21:09:40, rajith attapattu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2364/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-12 21:09:40)
bq.
bq.
bq. Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
bq. (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
bq.
bq. Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
bq.
bq. In summary the changes are,
bq. 1. The code now uses AddressBasedDestination if the syntax is ADDR.
bq. 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
bq. 3. The code also delegates creating of subscriptions.
bq.
bq. TODO.
bq. 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
bq. 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
bq.
bq. (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
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/BasicMessageProducer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
bq.
bq. Diff: https://reviews.apache.org/r/2364/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
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
>
>
> 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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127710#comment-13127710 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, line 976
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line976>
bq. >
bq. > This new isTopic(Destination) call doesnt seem like an improvement. If a destination is a Topic, then it should implement the Topic interface and the existing check is sufficient.
bq. >
bq. > Looking at the implementation of the isTopic method in AMQDestination, it seems questionable since its perfectly possible for queues bound to amq.topic not to be getting consumed from as JMS Topics.
bq. > If a destination is a Topic, then it should implement the Topic interface and the existing check is sufficient.
Rajith: This is actually not true. When a destination is created using a String, you cannot figure out if it's a "Topic" or a "Queue" until it's been resolved.
So we can only return an Object implementing the javax.jms.Destination !
Please note the existing destination implementation is not entirely correct in it's interpretation of the concept of Topics and Queues as it's quite narrow.
Simply bcos a queue is bound to amq.direct does not make it a "Queue' nor does it make it a Topic bcos it's bound to 'amq.topic'.
bq. > Looking at the implementation of the isTopic method in AMQDestination, it seems questionable since its perfectly possible for queues bound to amq.topic not to be getting consumed from as JMS Topics.
The AddressBasedDestination overrides the isTopic() and isQueue() methods to identify if address resolves to a Queue or a Topic.
What's missing is that if the underlying QpidDestination is null it should resolve it and provide the correct answer.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, lines 1037-1059
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line1037>
bq. >
bq. > Is this condition being used as an equivelent to if(BURL)else(ADDR)?
bq. >
bq. > This looks like it is in need of some abstraction.
It is infact the same. I used AMQTopic and AddressBasedTopic to make it more explicit.
However I dislike the whole thing as I believe we need a more unified implementation w.r.t Destinations.
As mentioned I didn't go as far I could have, but on the hand I am not sure how far we can go either with the current code base.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, lines 1110-1121
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line1110>
bq. >
bq. > Repeated if(AMQTopic)else() conditions, could use abstraction.
bq. >
bq. > Can the existing methods for exchangeName and queueName be overriden in the AddressBasedTopic implementation?
I've since done that, but forgot to add it there. The AddressBasedDestination does have these method overriden.
I will update the patch.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, lines 1250-1284
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line1250>
bq. >
bq. > Candidate for some sort of Destination factory?
Certainly !
The goal was to have minimal impact on the respective AMQxxx classes during the first phase.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, line 2515
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line2515>
bq. >
bq. > The old way seems better. If I add another Topic implementation later on, I'd now have to track this line down and update it.
correct ! The idea was to restrict Topics to the two said implementations to keep it simple until we sort out a proper abstraction at the AMQxxx class level.
I'd expect changes that should make most of the above code obsolete.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, line 149
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line149>
bq. >
bq. > This doesnt seem to be used?
correct - forgot to remove this.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, lines 580-588
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line580>
bq. >
bq. > Why does this catch an Exception now when it didnt before? Also, Exception rather than something more specific?
The underlying address based implementation could throw an exception here.
As for the exception handling and other changes, I do have further improvements in mind.
This patch is the beginning of a series of improvements I have in mind.
I want to take a step by step approach.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, line 598
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line598>
bq. >
bq. > As per previous comments on the isTopic() changes in AMQSession, this condition seems questionable. If a Topic destination wasnt provided, then overriding what it is classed as based on the exchange in use seems invalid. Also, this looks to clash with logic used in BasicMessageConsumer which decides the value of preAcquire in a different way (which is in itself a bug we were already aiming to fix by consolidating those types of decision into 1 place).
This was existing code which I left as it is.
Perhaps I missed your point ?
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, line 1082
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line1082>
bq. >
bq. > It seems like at least some of these removed methods were better placed where they were.
bq. >
bq. > It doesnt seem unreasonable for the protocol-specific implementation of operations depending solely on the session to actually be in the protocol-specific Session implementations. Better here than in the Destination as some of the functionality now appears to be.
I provided an explanation for this in the design discussion part.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java, line 536
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49770#file49770line536>
bq. >
bq. > All the casting here seems ugly, and this is an operation ultimately performed by the session so it would seem cleaner if its implementation was ultimately part of the session.
The ugly casting here is due to the lack of a proper abstraction.
I have commented on the above and the specifics about Destinations being smart objects under the design discussion.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java, line 173
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49771#file49771line173>
bq. >
bq. > Whitespace error introduced (along with a vast number of others) despite no other change around this code.
Oh dear ! I have not being able to get rid of this issue in eclipse :(
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java, lines 229-230
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49771#file49771line229>
bq. >
bq. > TODO ?
Yes, correct. I wanted to achieve the same here without the destination object having to leak address specific details outside of the abstraction.
I wasn't planning to commit with these TODO items and the durable subscription stuff.
The aim was to attach a subsequent patch this weekend and put up what I had for review as early as possible.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2584
-----------------------------------------------------------
On 2011-10-12 21:09:40, rajith attapattu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2364/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-12 21:09:40)
bq.
bq.
bq. Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
bq. (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
bq.
bq. Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
bq.
bq. In summary the changes are,
bq. 1. The code now uses AddressBasedDestination if the syntax is ADDR.
bq. 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
bq. 3. The code also delegates creating of subscriptions.
bq.
bq. TODO.
bq. 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
bq. 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
bq.
bq. (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
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/BasicMessageProducer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
bq.
bq. Diff: https://reviews.apache.org/r/2364/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127715#comment-13127715 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
bq. On 2011-10-13 16:36:12, Andrew Kennedy wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AddressBasedDestination.java, line 100
bq. > <https://reviews.apache.org/r/2366/diff/1/?file=49774#file49774line100>
bq. >
bq. > I assume this and similar mean resolveAddress? Also, is there any particular reason why you can't go ahead and attempt address resolution here? Or is this actually a case of an unresolvABLE address, when destination is null? In the client, resolveAddress, create and azzert would be called sequentially, normally, right?
Since we do have access to the session, I see no reason why we could not go ahead an attempt to resolve the address. On the other hand, this check was there to catch programmer errors than a user error. That is an attempt to call create (azzert, delete ..etc) without explicitly resolving the address.
bq. From a users pov this will never happen as they don't really interact with the Destination object directly.
I don't feel strongly about either strategy.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2366/#review2551
-----------------------------------------------------------
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-subscribe@qpid.apache.org
[jira] [Updated] (QPID-3401) Refactor address resolution code
Posted by "Alex Rudyy (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Alex Rudyy updated QPID-3401:
-----------------------------
Attachment: model2.gif
> 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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127907#comment-13127907 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
bq. On 2011-10-14 13:46:19, Robbie Gemmell wrote:
bq. > Alex and I have given the 2 posts a look over. This review represents both our thoughts and contains comments on https://reviews.apache.org/r/2364 and https://reviews.apache.org/r/2366
bq. >
bq. > The lack of proper testing is for us a barrier to these changes being made so close to the release process beginning, whether the code appeared OK or not. The level of change here is fairly significant to be making this close to the release process without proper confidence through testing, however the current testing does not give that confidence and has thus far proven woefully inadequate. Over 18 months since the current Addressing syntax implementation was added, we are still spotting numerous severe issues relating to its use, none of which are caught by the existing tests and so may or may not still exist in this new/refactored implementation. For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
bq. >
bq. > There is a complete lack of unit tests for the Addressing code, both for the current and new implementations. We should be aiming to maximize the amount of unit testing we have, as they are faster to run than system tests, can be much more specific/targeted, and help make it clearer what is and isn't being tested. This must be rectified before commit, not after.
bq. >
bq. > That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after. Does that functionality work in the existing implementation, and are there any tests for it?
bq. >
bq. > Further to the concerns around testing, the next biggest concern would be the new design itself. For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements.
bq. >
bq. > The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist. Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.
bq. >
bq. > For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?
bq. >
bq. > Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.
bq. >
bq. > It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues).
bq.
bq. rajith attapattu wrote:
bq. First up, thanks for taking the time to look at the patches. I appreciate it.
bq. As for the testing situation I agree that the current suite doesn't adequately cover the area in question. This is a topic that I was hoping to tackle after the release. In fact our current test suite doesn't seem to inspire confidence in many situations. As for the comment on unit testing. I will ensure that there is adequate coverage where is make sense. I will put up a patch for review.
bq. I also hear your concern about this being done close to the release and have taken that into account in deciding to hold on to it until the trunk is open again.
bq.
bq. >>For example, just by looking at the Addressing code while doing other work, it was recently spotted that rollback and recover are broken when using Address based Topics.
bq. Could you please provide more details here? Is there a JIRA regarding this? I will investigate this issue and ensure it's covered in the new implementation.
bq.
bq. >>That some functionality related to durable subscriptions is known not to work would also seem to require rectification before this is committed rather than after.
bq. Sorry about the confusion. What I meant was that durable subscriptions will not work with this patch and additional work is required. My intention was to post a follow up patch and as a separate review. I held back as there was already two reviews in progress.
bq. However this patch will only be committed alongside the durable subscription patch.
bq.
bq. >>Does that functionality work in the existing implementation, and are there any tests for it?
bq. It does work with the existing functionality. testDurableSubscriber() in AddressBasedDestinationTest.java
bq. However we should have destination syntax independent tests so we could leverage the existing durable subscriber tests (all though they themselves are a bit thin).
bq. In fact in general our test suite should be able to be independent of destination, sessions, connection implementations etc..
bq.
bq. I will address your comments related to the design in a separate comment.
bq.
bq.
bq. rajith attapattu wrote:
bq. Let me discuss your points on the design here. Actually this was one of the goals of putting this up for review. I want to ensure we get our design right.
bq.
bq. <quote> For a second time we seem to lack the ability to abstract common behaviour relating to our Destinations when using the differing syntax, providing the ability to isolate syntax related operations into methods which can be invoked within the various JMS operation implementations like consumer creation, producer creation etc. Instead, we continue to have a multitude of if(BURL)[else(FOO)] and if(ADDR)[else(FOO)] statements. </quote>
bq.
bq. I am glad you raised this point. All though not clearly visible with the patch as it doesn't go that far, this is in fact 'the' main goal of the re-factoring.
bq.
bq. If you look closely org.apache.qpid.messaging.address.amqp_0_10 contains an <b> 0-10 specific </b> implementation of the <b> address based </b> implementation of a "QpidDestination".
bq.
bq. (*) You could also have other syntax based implementations or,
bq. (*) Address based implementations for various protocol versions.
bq.
bq. The rest of the code will be working with QpidDestination interface (or QpidTopic & QpidQueue where necessary) without having to have any knowledge about how the destination was constructed (using a particular syntax) or the finer details of how to create/assert/delete (or even sending and receiving) for a specific protocol version for the respective Destination.
bq.
bq. Unfortunately given the time constraints and in order to keep the scope relatively small, I didn't go that far (a fact I clearly mentioned in the description).
bq. I intend to do this step by step.
bq.
bq. Therefore I ended up bridging the new abstraction with the AMQxxx classes via the AddressBasedDestination class. The various AMQxxx classes makes it quite difficult to get to that state without significant work. So I ended up with the bridging mechanism and the dreaded if(BURL)[else(FOO)] for the time being.
bq.
bq.
bq. <quote>The new Destination objects having particular implementations for creating and deleting queues etc does not seem like the most appropriate structure, i.e. Destinations don't create queues, they are queues. Things that use Destinations such as Sessions create them, and that is also where the operations to do so actually exist.</quote>
bq.
bq. I quite disagree with this assessment. The main goal here was to provide a clean abstraction to the rest of the code via the QpidDestination interface without them having to worry about syntax and protocol specifics. It's the underlying "protocol session" that contains the "protocol method" to create/delete queues not really your high level Session implementation. In fact from AMQP 1.0 the protocol session no longer has these methods.
bq.
bq. If you make the Destinations fairly dumb then you end up complicating your session implementations.
bq. Your session implementation will need to know the specifics for a particular syntax (links, nodes ..etc) and also the protocol specifics in order retrieve information from the destination and invoke the protocol specific methods.
bq.
bq. If you want to support multiple syntaxes (our current situation) then you end up with the dreaded if (BURL) situation.
bq. One only needs to look at the AMQSession.java , AMQSession_0_8.java and AMQSession_0_10.java to see why it's a bad idea to have high level session implementations that are protocol specific.
bq.
bq. IMO the destinations should be immutable and is smart enough to know what it needs to do and how to do it.
bq.
bq. <quote>Doing this gives the Destinations far too much intimate knowledge of the underlying implementation, making them harder to maintain and more difficult to test.</quote>
bq.
bq. I am not quite sure what you meant here. Could you elaborate more here on how it makes testing difficult? (keeping in mind my above explanation)
bq.
bq.
bq. <quote>It could also easily be argued that Destination objects should be immutable. That it is possible to create a Destination using the JMS API or a properties file from what is effectively just a String, and that this String value is sufficient to identify the Destination for use by someone else, suggests the level of mutating operations we currently have in our Destination implementations is rather incorrect (and also creates scope for thread safety issues). </quote>
bq.
bq. IMO Destinations should be immutable once it's created from a string (or more broadly a specification, where the simplest form being a string) !
bq. If you need to say create a copy, you could do deepCopy() with specific parameters rather than having setters on the Destination.
bq.
bq. Creating a durable subscription is a good example here. One reason why I wanted to submit as a separate patch.
bq. I wanted a way to create the new Topic from the given topic with durability, but not by invoking a setter like setDurable on the newly created Topic.
bq.
bq. However I couldn't go that far in a single step due to the way our current code works.
bq.
bq. rajith attapattu wrote:
bq. <quote>Not solely specific to the redesign, it seems like the Address resolution is currently performed on a global client basis for a given Destination, which doesn't seem sufficient. The existence of a Destination on one Connection doesn't provide any resolution guarantees for the same Destination when used on another Connection later, which suggests Destinations must instead be resolved on a per-Connection basis. These Connections could be to entirely different brokers for example, or the broker may have been restarted, failover could have occurred to another broker, or administrative changes may have altered the broker state such that the previous resolution is no longer accurate when the Destination is reused.</quote>
bq.
bq. I agree with you about Destinations being resolved per underlying protocol connection (not the high level JMS destination) as administrative (or other) changes may have made the previous resolution invalid. Infact a Destination should be resolved every time it's being used to create a producer or consumer.
bq.
bq. However due to a deadlock issue this was difficult to do in the current code base (see QPID-2808)
<quote>For all the new Destination related code being added, there doesnt appear to be any removal of the previous Addressing code added when the first implementation was done. Surely this change leaves us with substantial amounts of dead code lying around, which needs to be cleaned up?</quote>
Existing code that is not needed are indeed removed. I haven't included them in the patches for review to keep the diff simple. Have a look at the QPID-3401.patch attached to the JIRA to get a sense of all the code that has been removed.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2583
-----------------------------------------------------------
On 2011-10-12 21:09:40, rajith attapattu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2364/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-12 21:09:40)
bq.
bq.
bq. Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
bq. (Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
bq.
bq. Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review [ https://reviews.apache.org/r/2366/ ] to ensure clarity.
bq.
bq. In summary the changes are,
bq. 1. The code now uses AddressBasedDestination if the syntax is ADDR.
bq. 2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
bq. 3. The code also delegates creating of subscriptions.
bq.
bq. TODO.
bq. 1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
bq. 2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
bq.
bq. (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
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/BasicMessageProducer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
bq.
bq. Diff: https://reviews.apache.org/r/2364/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13271871#comment-13271871 ]
jiraposter@reviews.apache.org 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: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "Rajith Attapattu (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13125876#comment-13125876 ]
Rajith Attapattu commented on QPID-3401:
----------------------------------------
The following set of patches aims to refactor the addressing code to achieve the following goals.
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.
All though the patch looks substantial, most of the size comes from
(1) Addition of new classes (and most of the code was existing code moved into more structured classes).
(2) Deletion fo unwanted code/classes.
The new code hooks into the client code at the following locations.
{code}
BasicMessageConsumer_0_10.java -> constructor() - to create & azzert
-> cleanupQueue() - to delete
* // ideally we should be invoking addrDest.createSubscription() in the constructor;
* // ideally we should be invoking addrDest.deleteSubscription() in the close();
BasicMessageProducer_0_10.java -> declareDestination() - to create & azzert
-> close() - to delete
-> send() - used the AddressBasedDestination to retrieve some address specific info.
* // We could use a send() method on a destination to abstract the protocol specific and destination syntax specific details.
// I didn't go that far during the refactoring.
AMQSession_0_10.java -> sendConsume() - to createSubscription
{code}
The following diagram describes the new class structure. https://issues.apache.org/jira/secure/attachment/12498753/class_diagram.png
I have produced separate patches for each module to keep it simple.
> 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: class_diagram.png
>
>
> 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-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13248234#comment-13248234 ]
jiraposter@reviews.apache.org 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: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ 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
[jira] [Commented] (QPID-3401) Refactor address resolution code
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126142#comment-13126142 ]
jiraposter@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/
-----------------------------------------------------------
Review request for Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
Summary
-------
The following is a patch that illustrates the changes made to the core client namely the session, message consumer and producer classes.
(Please note that in order to compile and run the tests you need to get apply the QPID-3401.patch attached to the JIRA.)
Most of the code removed from the AMQSession_0_10.java have been included in the new class structure posted as a separate review to ensure clarity.
In summary the changes are,
1. The code now uses AddressBasedDestination if the syntax is ADDR.
2. For address destinations the code now delegates the creation, assertion, deletion actions to the underlying QpidDestination class via the AddressBasedDestination.
3. The code also delegates creating of subscriptions.
TODO.
1. Delegate the deleting of subscriptions (minor change which will follow once this patch is approved)
2. Currently Durable Subscribers want work with AddressBasedDestinations (This will be done in a follow up patch that will be posted soon).
(AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes are included along with the new class structure patch as a separate review).
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/BasicMessageProducer_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1182391
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java 1182391
Diff: https://reviews.apache.org/r/2364/diff
Testing
-------
All existing tests in AddressBasedDestination test pass (with the exception of the Durable subscription test).
Thanks,
rajith
> 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: class_diagram.png
>
>
> 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-subscribe@qpid.apache.org
[jira] [Updated] (QPID-3401) Refactor address resolution code
Posted by "Rajith Attapattu (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Rajith Attapattu updated QPID-3401:
-----------------------------------
Attachment: QPID-3401.patch
QPID-3401-systests.patch
The systests patch isolates the changes made to the test cases.
The QPID-3401.patch contains all changes necessary to compile and run the tests.
The main code changes are submitted for review under
https://reviews.apache.org/r/2366/
https://reviews.apache.org/r/2364/
> 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
>
>
> 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-subscribe@qpid.apache.org
[jira] [Updated] (QPID-3401) Refactor address resolution code
Posted by "Rajith Attapattu (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Rajith Attapattu updated QPID-3401:
-----------------------------------
Fix Version/s: (was: 0.14)
0.15
> 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.15
>
> 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-subscribe@qpid.apache.org