[ 
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

Reply via email to