[
https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127553#comment-13127553
]
jirapos...@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-subscr...@qpid.apache.org
