----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/860/#review801 -----------------------------------------------------------
/branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableQueue.h <https://reviews.apache.org/r/860/#comment1747> Ok, discussed this with Kim - his understanding of this pointer is that it is owned by the persistent queue, and the queue should be responsible for calling delete on it. If that is the case, then there is a bug in that the store is referencing this context after the queue is destroyed. I will work with Kim on this to better understand the root cause and fix it properly. /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/860/#comment1749> "All you need to know is when they all complete" - not true: we need to know when _each_ dequeue completes. This is not like the enqueue case - we need to known when each dequeue is complete as the Message.Accept may be only interested in that one particular dequeue event. Which is what QPID-3079 is specifically trying to solve. /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp <https://reviews.apache.org/r/860/#comment1748> Because of this: https://bugzilla.redhat.com/show_bug.cgi?id=698721 Store will not complete the dequeue for up to one second *if* it doesn't have enough work to perform immediately. Broker needs to force fast completion on Execution.Sync - it does this by issuing a flush() on the command. And that's why a simple counter won't work here - we need to track the queues that are in the process of dequeuing specifically in case a flush is called. /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp <https://reviews.apache.org/r/860/#comment1750> Which is what I'm trying to do - at least at the SemanticState level (with the message accept context). Its just that the message.accept context has to be able to track N outstanding dequeues in order to know when it is complete (could be a counter), or if the SessionState flushes the command (counter not sufficient - it needs list of messages or queues to flush). Let me step back a bit - there are three layers of abstraction introduced by this patch: 1) SessionState/SessionContext needs to allow for async commands with an abstract api (i.e. - SessionState doesn't care about the command specifics, it just needs to know when it is complete, and be able to flush the command if needed) See changes in SessionContext. 2) SemanticState uses one of these abstract "async command contexts" for the Message.Accept command. This context must monitor a _set_ of outstanding dequeues in order to 1) know when the command is complete, and 2) which queues/messages need flushing should SessionState requires a flush. 3) The Queue needs to provide a callback when a particular message is dequeued (not when all dequeues complete for a given message). The callback needs to identify the queue and the particular message that has completed dequeue. #3 could be tracked by the message instead, but we still need to know when -each- dequeue completes, so then the message would have to track each of its concurrent dequeue's state (and target queue). I think that's exactly what I've got Queue doing now anyways, and I think representing that in the Queue is more "natural" - but that's more my artistic opinion :) - Kenneth On 2011-06-08 20:31:05, Kenneth Giusti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/860/ > ----------------------------------------------------------- > > (Updated 2011-06-08 20:31:05) > > > Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet. > > > Summary > ------- > > Modifies the broker's handling of Message.Accept to hold off the completion > of the command until all messages related to the accept have completed > dequeue. This particularly applies to persistent messages, as the > store::dequeue() operation must complete before the message is considered > fully dequeued. > > Note this bugfix requires some changes to the broker's store module > interface: previously, the store only identified the message when a dequeue > was completed. This is not enough information - the queue from which is was > removed must also be identified (the message may be in the process of being > dequeued on several queues at once). > > > This addresses bug qpid-3079. > https://issues.apache.org/jira/browse/qpid-3079 > > > Diffs > ----- > > /branches/qpid-3079/qpid/cpp/src/qpid/broker/AsyncCompletion.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Message.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Message.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStore.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStoreModule.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStoreModule.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/NullMessageStore.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/NullMessageStore.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableQueue.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/RecoverableQueue.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp > 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionContext.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionState.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionState.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/tests/AsyncCompletion.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/tests/QueueTest.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/tests/TestMessageStore.h 1124895 > > Diff: https://reviews.apache.org/r/860/diff > > > Testing > ------- > > broker unit tests, store unit tests (modified jboss store). Still needs to > be vetted on non-linux, and have latest trunk merged in. > > > Thanks, > > Kenneth > >
