----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/860/#review795 -----------------------------------------------------------
/branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.h <https://reviews.apache.org/r/860/#comment1713> Why indirection via a factory? Why not just pass callback pointer? /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp <https://reviews.apache.org/r/860/#comment1714> Why can't we do a map lookup here by queue name? Why the search by PersistenceId? /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp <https://reviews.apache.org/r/860/#comment1715> Looks like meaning of synclist has been reversed here - old code added to list on dequeue, new code removes from list. Is that right? /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h <https://reviews.apache.org/r/860/#comment1716> why the indirection via a factory? /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h <https://reviews.apache.org/r/860/#comment1717> Overloading operator () is just confusing. Give it a function name. /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/860/#comment1718> Do we need a map here? Could we store the callback directly on the PersistentMessage? /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/860/#comment1719> This would be simpler if the callback was stored on the message rather than in a map /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp <https://reviews.apache.org/r/860/#comment1720> Do we need the uniqueness logic? How often are we likely to see duplicate queues in the list? Flushing is an idempotent operation, so flushing twice would be an error. /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp <https://reviews.apache.org/r/860/#comment1721> Could this callback be attached to the message instead, avoiding heap allocation? /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp <https://reviews.apache.org/r/860/#comment1722> I think that using a factory to avoid construction of the callback is more expensive than always constructing the calback, especially if the callback is made part of the Message so there's no heap allocation. /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp <https://reviews.apache.org/r/860/#comment1723> Possible cluster issue. The cluster replicates the unacked list. Is this change predictable for the cluster? - Alan On 2011-06-08 20:31:05, Kenneth Giusti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/860/ > ----------------------------------------------------------- > > (Updated 2011-06-08 20:31:05) > > > 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/AsyncCompletion.h 1124895 > /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/Message.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Message.cpp 1124895 > /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/NullMessageStore.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/NullMessageStore.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableQueue.h 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/RecoverableQueue.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp > 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionContext.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionState.h 1124895 > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionState.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/tests/AsyncCompletion.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/tests/QueueTest.cpp 1124895 > /branches/qpid-3079/qpid/cpp/src/tests/TestMessageStore.h 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 > >
