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