On 09/15/2009 02:13 PM, Carl Trieloff wrote:
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?
Setting it in message builder is sufficient.
(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
Create a durable queue with an alternate exchange specified and a second
durable queue with a flow to disk policy specified. Bind both to a
fanout exchange and send enough messages to that exchange to trigger
flow to disk for the second queue, releasing message content. Bind a
third queue - transient - to the alternate exchange for the first queue.
Delete the first queue (this will route the messages through the
alternate exchange and enqueue them in the third queue). Consume the
messages from the second queue.
However, removing the setStore() call from the Queue::push() would
resolve the issue.
* 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.
I'm not sure I agree. Flow to disk is not supported on a non-durable
queue; if you ask for it you will instead get a reject policy. That will
be in effect for every message enqueued on that queue, regardless of the
routing.
The case where you have a message enqueued on multiple queues and don't
want to release it from memory is an entirely different case. This case
is dependent on the number of other matching bindings for the published
message.
We also have the issue that
our publish to multiple queues is not atomic outside a txn.
Checking that all queues are durable beforehand does nothing to change
that though.
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.
As above I'm not convinced that any option is more 'consistent' than any
other. The root issue is where the 'policy' on one queue could affect
other queues with matching bindings and how to deal with that.
The real solution to that is a bigger job, but the most urgent
manifestation of this is where releasing the content of the shared
message due to a policy on one queue prevents that message being
delivered on another.
My view would be that we simply don't release the content if we
determine that not all the queues the message is enqueued on will be
able to handle that. I think that will result in a cleaner short term
fix for the immediate problem.
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.
I agree a clean set of changes is the goal, living with those design
defects we cannot remedy immediately but trying hard not to make things
any more incoherent.
I don't mind how many Jiras we use to track the issues. I have created
two already, but don't mind if those get augmented or replaced,
providing we make the problems being solved clear.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
