[ 
https://issues.apache.org/jira/browse/QPID-3079?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13046784#comment-13046784
 ] 

jirapos...@reviews.apache.org commented on QPID-3079:
-----------------------------------------------------


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/820/#review747
-----------------------------------------------------------



/branches/qpid-3079/qpid/cpp/src/qpid/broker/AsyncCompletion.h
<https://reviews.apache.org/r/820/#comment1522>

    why virtual inheritance?
    



/branches/qpid-3079/qpid/cpp/src/qpid/broker/AsyncCompletion.h
<https://reviews.apache.org/r/820/#comment1523>

    Why clone?
    



/branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.h
<https://reviews.apache.org/r/820/#comment1524>

    yuck
    



/branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp
<https://reviews.apache.org/r/820/#comment1526>

    Why iterate here rather than doing a map lookup?



/branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h
<https://reviews.apache.org/r/820/#comment1527>

    Why the indirection via a factory?



/branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h
<https://reviews.apache.org/r/820/#comment1528>

    suggest named function rather than overloading operator ()



/branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h
<https://reviews.apache.org/r/820/#comment1529>

    New map, performance?
    Safe to have PersistableMessage*?



/branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp
<https://reviews.apache.org/r/820/#comment1532>

    Possible cluster issue: map sorted by pointers.



/branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp
<https://reviews.apache.org/r/820/#comment1533>

    Instead of a map, can we put a shared_ptr<DequeueDoneCalback> in 
PersistableMessge?



/branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp
<https://reviews.apache.org/r/820/#comment1534>

    Need a less obscure error message.



/branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp
<https://reviews.apache.org/r/820/#comment1537>

    Can we use a count instead of a map?
    



/branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp
<https://reviews.apache.org/r/820/#comment1535>

    Or:
    
    assert(unique);
    (void)unique; // no op but takes care of unused variable warnings.



/branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp
<https://reviews.apache.org/r/820/#comment1536>

    Replace these 3 lines with: pending.erase(id)



/branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp
<https://reviews.apache.org/r/820/#comment1538>

    trace, not error



/branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp
<https://reviews.apache.org/r/820/#comment1539>

    We don't need to do anything in the TX case?
    



/branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp
<https://reviews.apache.org/r/820/#comment1540>

    not sure


- Alan


On 2011-06-01 20:19:53, Kenneth Giusti wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/820/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2011-06-01 20:19:53)
bq.  
bq.  
bq.  Review request for Alan Conway and Gordon Sim.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  Here's my attempt to implement a solution to qpid-3079 - with an eye 
towards allowing async completion of various commands.  I've integrated the 
existing async Message.Transfer completion code to use this new design.
bq.  
bq.  The implementation seems too "codey" - I had hoped that this would've 
resulted in a simpler implementation, but I've found that C++ is a harsh 
mistress.
bq.  
bq.  I'd like some early feedback - particularly suggestions for reducing the 
amount of code/simplifying the design if possible.
bq.  
bq.  There are two major functional changes involved:
bq.  
bq.  1) changes to the Queue:dequeue() method to allow a callback if the 
dequeue is asynchronous.  Since the caller does not know ahead of time whether 
the dequeue is sync or async, Queue::dequeue() now takes a functor parameter 
supplied by the caller.  The purpose of the functor is to allocate a context to 
track the dequeue completion only if necessary.  See Queue.h/cpp and the 
SemanticState::accepted() changes for detail
bq.  
bq.  2) - and this is the one aspect I'd like to improve - changes to the 
SessionState/SessionContext/SemanticState interface to allow a command to 
complete asynchronously.   My understanding of the code is that the 
SessionContext defines an abstract interface to the SessionState.  The 
SemanticState uses this interface for getting session-related information.
bq.  
bq.  The changes involve creating an abstract class within the SessionContext 
class that represents the context for a single outstanding asynchronous 
command.  This class is abstract - the intent is for SemanticState to subclass 
it and add any state it needs in order to asynchronously process a particular 
command (see the class AsyncMessageAcceptCmd in SemanticState for an example of 
a derivation used for asynchronous Message.Accept).  My intent was to have 
SemanticState allocate one of these contexts should a command require async 
completion - and use this context to retain state while the command executes.
bq.  
bq.  Then the SemanticState would register the context with the SessionState 
via the SessionContext's registerAsyncCommand() method.  The SessionContext 
would store information that it needs when the command completes (right now: 
sequence #, isSync bit set, and acceptRequired in the case of message.transfer).
bq.  
bq.  What I particularly do not like is the exposure of the AsyncCommandManager 
context into the SessionContext interface.  This command manager was created 
when I added support for async Message.Transfer - it provided a context that 
would track all outstanding message.transfer commands that were in flight 
should the SessionState be destroyed.   The command manager context is 
reference-counted, and each outstanding command references it - guaranteeing 
that some context is present when the command completes, even if the session 
has been destroyed.
bq.  
bq.  It would be great if I could hide this manager context from being exported 
by SessionContext.  And I'd like a better way of tracking the command seq #, 
isSync, and requiresAccept rather than just jamming them into the command 
context when it is registered...
bq.  
bq.  Regardless, please give me your impressions before I go too far down the 
testing path, if possible.   Thanks.
bq.  
bq.  
bq.  This addresses bug qpid-3079.
bq.      https://issues.apache.org/jira/browse/qpid-3079
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/AsyncCompletion.h 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.h 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/Message.h 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/Message.cpp 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStore.h 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStoreModule.h 
1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStoreModule.cpp 
1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/NullMessageStore.h 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/NullMessageStore.cpp 
1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.h 
1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp 
1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableQueue.h 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionContext.h 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionState.h 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionState.cpp 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/tests/QueueTest.cpp 1124901 
bq.    /branches/qpid-3079/qpid/cpp/src/tests/TestMessageStore.h 1124901 
bq.  
bq.  Diff: https://reviews.apache.org/r/820/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  None - it compiles.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Kenneth
bq.  
bq.



> 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