[ https://issues.apache.org/jira/browse/QPID-3079?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13051293#comment-13051293 ]
jirapos...@reviews.apache.org commented on QPID-3079: ----------------------------------------------------- ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/934/ ----------------------------------------------------------- Review request for qpid, Gordon Sim, Kim van der Riet, and Steve Huston. Summary ------- In order to solve QPID-3079, I'd like to make the following changes to the MessageStore API. The problem with the existing api is that, when a dequeue() is completed, only the message is notified (via a call to PersistableMessage::dequeueComplete()). Since a single instance of a message may be dequeued from multiple queues simultaneously, this approach does not provide enough context to allow the broker to identify -which- dequeue is being completed. The proposed solution is to provide a new method to the PersistableQueue class that is called by the store when a dequeue completes: virtual void dequeueComplete(const boost::intrusive_ptr<PersistableMessage>&) = 0; This allows the broker to identify both the queue that is the source of the dequeue, plus the message that was dequeued. I've also modified the MessageStore::dequeue() interface to take a shared_ptr to the queue, much like the intrusive_ptr that is currently used to pass the message. This allows an asynchronous dequeue to hold a valid pointer to the queue until the dequeue completes. (Gordon has suggested that I make the same changes to the MessageStore::enqueue() interface for symmetry - something that I'd like to do if there is agreement). All changes for QPID-3079 are available on the qpid-3079 branch in svn. The changes included in this review are merely the store-related modifications - I wanted to focus on these separately. The full review of QPID-3079 is available here: https://reviews.apache.org/r/860/ - and feel free to checkout the branch if you'd like :) This addresses bug qpid-3079. https://issues.apache.org/jira/browse/qpid-3079 Diffs ----- /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/PersistableQueue.h 1124895 /branches/qpid-3079/qpid/cpp/src/qpid/store/MessageStorePlugin.h 1124895 /branches/qpid-3079/qpid/cpp/src/qpid/store/MessageStorePlugin.cpp 1124895 /branches/qpid-3079/qpid/cpp/src/qpid/store/StorageProvider.h 1124895 /branches/qpid-3079/qpid/cpp/src/qpid/store/ms-clfs/MSSqlClfsProvider.cpp 1124895 /branches/qpid-3079/qpid/cpp/src/qpid/store/ms-sql/MSSqlProvider.cpp 1124895 Diff: https://reviews.apache.org/r/934/diff Testing ------- I've tested these changes against the Linux-based store implementation. I've only verified that the sql stores build under windows thus far - Steve, are there any unit tests for these stores? If so, is this something I can run? Otherwise, could you try running them from the qpid-3079 branch and let me know if there are failures? Thx. Thanks, Kenneth > message.accept command should be completed on a per-dequeue basis > ----------------------------------------------------------------- > > Key: QPID-3079 > URL: https://issues.apache.org/jira/browse/QPID-3079 > Project: Qpid > Issue Type: Bug > Components: C++ Broker > Affects Versions: 0.8, 0.9 > Reporter: Ken Giusti > Assignee: Ken Giusti > Fix For: 0.11 > > Attachments: proposal.txt > > > ** Overview > Asynchronous completion means that command execution is initiated in one > thread > (a client connection thread) and completed in a different thread. > When the async store is loaded, message.transfer commands are > completed by a store thread that does the async write. > ** Issues with asynchronous completion code as of revision r1029686 > *** Not really asynchronous > IncompleteMessageList::process blocks the connection thread till all > outstanding async commands (messages) for the session are complete. > With the new cluster, this could deadlock since it is blocking a Poller > thread. > *** Race condition for message.transfer > > Sketch of the current code: > // Called in connection thread > PersistableMessage::enqueueAsync { ++counter; } > // Called in store thread once message is written. > PersistableMessage::enqueueComplete { if (--counter == 0) notifyCompleted(); } > The intent is that notify be called once per message, after the > message has been written to each queue it was routed to. > However of a message is routed to N queues, it's possible for > notifyCompleted to be called up to N times. The store thread could > call notifyCompleted for the first queue before the connection thread > has called enqueueAsync for the second queue, and so on. > *** No asynchronous completion for message.accept > We do not currently delay completion of message.accept until the > message is deleted from the async store. This could cause duplicate > delivery if the broker crashes after completing the message but > before it is removed from store. > There is code in PersistableMessage to maintain a counter for dequeues > analogous to to the async enqueue code but this is incorrect. > Completion of transfer is triggered when all enqueues for a message are > complete. > Completion of accept is triggered for *each* dequeue from a queue > independently. > Furthermore a single accept can reference many messages, so it can't be > associated with a message. > ** New requirements > The new cluster design will need to participate in async completion, e.g. > an accept cannot be comlpeted until the message is > - removed from store (if present) AND > - replicated to the cluster (if present) as dequeued > The new cluster also needs to asynchronously complete binding commands > (declare, bind, delete) when they are replicated to the cluster. -- This message is automatically generated by JIRA. 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