Gordon Sim wrote:
On 09/11/2009 02:33 PM, [email protected] wrote:
Author: kpvdr
Date: Fri Sep 11 13:33:42 2009
New Revision: 813825
URL: http://svn.apache.org/viewvc?rev=813825&view=rev
Log:
Joint checkin with cctrieloff. Refactor of exchange routing so that
multi-queue policy differences may be resolved and allow for correct
multi-queue flow-to-disk behavior. Different queues may have
differing policies and persistence properties - these were previously
being neglected. New c++ test added.
I'm not very keen on some of these changes. While the original code is
already unpleasant, this allows the bad smells to leak further into
the codebase.
On a separate point, though its great to get whitespace fixed up, when
a commit like this one is contains >60% whitespace changes mixed up
with various real semantic changes it becomes much harder to review.
However, back to the more important issue, the actual changes and the
specific things I dislike:
* setting the store on a message in Queue::push()
This means that if a message previously pushed on to a durable queue
then gets pushed on to a transient queue, the store pointer will be
set to null.
Though that case could be fixed by a simple check this seems like the
wrong place to be associating the message with a store. In fact the
same line has also been added to the MessageBuilder (which I think is
a reasonable change), so its not clear why its called again here.
Agree -- passing it on release was worse. We should reason through if we
could pass it only in messageBuilder. Thoughts on that?
(And though checking whether all queues are persistent before routing
may mean that in the common cases overwriting the store causes no
harm, what about the case where a queue is deleted and its messages
are routed through an alternate exchange?).
Don't know. Are you able to suggest a test for this case, so that I can
understand what would be failing
* iterating through all matching bindings to check that the queues
have a persistence id and taking that to mean they are persistent and
a store is loaded before iterating over the same list again to route
each message
I find that a little ugly and it leaks aspects of the rather
unpleasant flow-to-disk 'feature' to the exchange class.
ack, however we have bit of an issue here which we need to deal with.
The basic case is, if we don't have a store module loaded, then the
flow-to--disk limits will be applied as reject. However if we can't flow
a message to disk when the store is loaded then we would just not flow
it to disk. This is totally inconsistent. We also have the issue that
our publish to multiple queues is not atomic outside a txn. That we need
to debate if we want to change that, but I don't see any way to resolve
the former without checking the queue state prior to routing. Any other
ideas welcome.
* using qpid::sys::CopyOnWriteArray<Binding::shared_ptr>::ConstPtr as
the type of the parameter to the new doRoute method on Exchange.
It would be far better to just use a const reference to a
Binding::vector. The CopyOnWriteArray logic is irrelevant to the
method in question, all that is being used is that typedef which just
makes someone reading the code work harder to figure out it is
irrelevant.
That is particularly the case in the Topic exchange for which the new
typdef qpid::sys::CopyOnWriteArray<Binding::shared_ptr>::Ptr is added
to CopyOnWriteArray for no apparent reason and used in this one place
(again when none of the CopyOnWriteArray logic is being used).
yes, binding vector would be better (CopyOnWriteArray is not used so no
point having it in the signature)
* perpetuating the unclear division of role between PersistableMessage
and Message itself; the logic for releasing and controlling the
release seems split between these two in what seems like an arbitrary
manner.
Note also that with this change transactionally published messages
will never be released from memory as the call to do so (if required)
in SemanticState will happen before any request has been made to
release the content.
The code for this aspect clearly has a number of problems which we
need to fix - creating Jiras for these would I think be valuable. I
think we need to try hard to get the cleanest possible set of changes
though, and prevent the already incoherent design from deteriorating
even further.
Some good observations - nice review, however I don't think we can
consider releasing trunk until this is cleaned up.
There are a few things that the patch does clean up:
- the massive code duplication in exchange route
- the consolidation of all the release state into one manager.
I do however agree that we might want to find a way to encapsulate the
release action into Persistable message thus removing the release from
SemanticState, which will also correct the txn case.
I would suggest that we correct the signatures on the route re-factor
part, the setting of store ptr & move the release logic all to the
manager & persistableMessage and we see what the patch looks like.
other notes:
Issues that need to be covered are:
- delaying release content till after processing queueEvents
- delaying release of content until all messages have been enqueued
- making behaviours consistent when a message can be flowed to disk
- stop using stage() for flow to disk messages
- correct ftd txn behaviour
- resolving the ability to be able to read directly on write
I think we can track this with one JIRA, as I think the issues need to
be worked together to be able to get a clean refactor which I believe is
the goal.
my 2 cents.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
