----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/860/#review859 -----------------------------------------------------------
Looking good, just a few suggestions. Note on testing: need to run store tests as well as qpidd tests. /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.h <https://reviews.apache.org/r/860/#comment1878> Why the change from dequeue to list? /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp <https://reviews.apache.org/r/860/#comment1888> Static locals not thread safe, use a normal local variable. /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp <https://reviews.apache.org/r/860/#comment1879> /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp <https://reviews.apache.org/r/860/#comment1880> What happens in the case isRedundant() == true? /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h <https://reviews.apache.org/r/860/#comment1881> Functions are too big to inline in .h, move to .cpp. /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h <https://reviews.apache.org/r/860/#comment1886> Suggest a change here to make it more flexible and type safe: registerCallback(boost::function<void()> f); You can package up all the details in boost::bind at the call site, this class doesn't need to know. See comments below /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h <https://reviews.apache.org/r/860/#comment1882> Store the DequeueCompletion pointer on the PersistableMessage, avoid this map lookup. /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/860/#comment1883> static local variables are not thread safe. Use a normal local variable constructing an intrusive pointer is trivial. /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp <https://reviews.apache.org/r/860/#comment1884> This doesn't need to be static, and you can avoid the casting. Just write void dequeueDone() { cmd->complete(); } and see further comment at the call site below /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp <https://reviews.apache.org/r/860/#comment1887> See comments above: this can now be async->registerCallback(boost::bind(&AsyncMessageAcceptCmd::dequeueDone, this)) - Alan On 2011-06-16 15:25:17, Kenneth Giusti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/860/ > ----------------------------------------------------------- > > (Updated 2011-06-16 15:25:17) > > > 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/DeliveryRecord.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp 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/SemanticState.cpp 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 > >
