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