[
https://issues.apache.org/jira/browse/QPID-3079?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13052155#comment-13052155
]
jirapos...@reviews.apache.org commented on QPID-3079:
-----------------------------------------------------
bq. On 2011-06-17 13:19:13, Alan Conway wrote:
bq. > Looking good, just a few suggestions.
bq. >
bq. > Note on testing: need to run store tests as well as qpidd tests.
Yes, both store and C++ broker unit tests are passing.
bq. On 2011-06-17 13:19:13, Alan Conway wrote:
bq. > /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp, line 116
bq. > <https://reviews.apache.org/r/860/diff/2/?file=21211#file21211line116>
bq. >
bq. > Static locals not thread safe, use a normal local variable.
Removed.
bq. On 2011-06-17 13:19:13, Alan Conway wrote:
bq. > /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp, line 125
bq. > <https://reviews.apache.org/r/860/diff/2/?file=21211#file21211line125>
bq. >
bq. > What happens in the case isRedundant() == true?
the D.R. is removed from the unacked list - this check has been moved to the
caller.
bq. On 2011-06-17 13:19:13, Alan Conway wrote:
bq. > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h, line 300
bq. > <https://reviews.apache.org/r/860/diff/2/?file=21213#file21213line300>
bq. >
bq. > Functions are too big to inline in .h, move to .cpp.
done.
bq. On 2011-06-17 13:19:13, Alan Conway wrote:
bq. > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h, line 310
bq. > <https://reviews.apache.org/r/860/diff/2/?file=21213#file21213line310>
bq. >
bq. > Suggest a change here to make it more flexible and type safe:
bq. >
bq. > registerCallback(boost::function<void()> f);
bq. >
bq. > You can package up all the details in boost::bind at the call site,
this class doesn't need to know. See comments below
Very nice - done.
bq. On 2011-06-17 13:19:13, Alan Conway wrote:
bq. > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp, line 665
bq. > <https://reviews.apache.org/r/860/diff/2/?file=21214#file21214line665>
bq. >
bq. > static local variables are not thread safe. Use a normal local
variable constructing an intrusive pointer is trivial.
done.
bq. On 2011-06-17 13:19:13, Alan Conway wrote:
bq. > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp, line 804
bq. > <https://reviews.apache.org/r/860/diff/2/?file=21215#file21215line804>
bq. >
bq. > This doesn't need to be static, and you can avoid the casting. Just
write
bq. >
bq. > void dequeueDone() { cmd->complete(); }
bq. >
bq. > and see further comment at the call site below
done.
bq. On 2011-06-17 13:19:13, Alan Conway wrote:
bq. > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp, line 966
bq. > <https://reviews.apache.org/r/860/diff/2/?file=21215#file21215line966>
bq. >
bq. > See comments above: this can now be
bq. >
bq. >
async->registerCallback(boost::bind(&AsyncMessageAcceptCmd::dequeueDone, this))
done.
bq. On 2011-06-17 13:19:13, Alan Conway wrote:
bq. > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h, line 437
bq. > <https://reviews.apache.org/r/860/diff/2/?file=21213#file21213line437>
bq. >
bq. > Store the DequeueCompletion pointer on the PersistableMessage, avoid
this map lookup.
Sorry Alan, I'm being dense here - I don't understand. Wouldn't the
PersistableMessage still need a container for the DequeueCompletions, as there
may be more than one dequeue pending for that message? I'm thinking of fanout
- same physical PersistableMessage shared among N queues. There may be several
different sessions dequeuing/accepting that message at the same time, right?
How do we 'map' (heh) back from the P.M. to the sessions pending on the
Message.Accept completion?
bq. On 2011-06-17 13:19:13, Alan Conway wrote:
bq. > /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.h, line 107
bq. > <https://reviews.apache.org/r/860/diff/2/?file=21210#file21210line107>
bq. >
bq. > Why the change from dequeue to list?
Big performance improvement with qpid-cpp-benchmark by moving from a dequeue to
a list. I'm guessing because we delete entries from the middle of the list(?)
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/860/#review859
-----------------------------------------------------------
On 2011-06-16 15:25:17, Kenneth Giusti wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/860/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-06-16 15:25:17)
bq.
bq.
bq. Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. 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.
bq.
bq. 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).
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/DeliveryRecord.h 1124895
bq. /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1124895
bq. /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp
1124895
bq. /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h 1124895
bq. /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp 1124895
bq. /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp 1124895
bq.
bq. Diff: https://reviews.apache.org/r/860/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. broker unit tests, store unit tests (modified jboss store). Still needs
to be vetted on non-linux, and have latest trunk merged in.
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
