> On 2011-06-17 13:19:13, Alan Conway wrote: > > Looking good, just a few suggestions. > > > > Note on testing: need to run store tests as well as qpidd tests.
Yes, both store and C++ broker unit tests are passing. > On 2011-06-17 13:19:13, Alan Conway wrote: > > /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp, line 116 > > <https://reviews.apache.org/r/860/diff/2/?file=21211#file21211line116> > > > > Static locals not thread safe, use a normal local variable. Removed. > On 2011-06-17 13:19:13, Alan Conway wrote: > > /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp, line 125 > > <https://reviews.apache.org/r/860/diff/2/?file=21211#file21211line125> > > > > What happens in the case isRedundant() == true? the D.R. is removed from the unacked list - this check has been moved to the caller. > On 2011-06-17 13:19:13, Alan Conway wrote: > > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h, line 300 > > <https://reviews.apache.org/r/860/diff/2/?file=21213#file21213line300> > > > > Functions are too big to inline in .h, move to .cpp. done. > On 2011-06-17 13:19:13, Alan Conway wrote: > > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h, line 310 > > <https://reviews.apache.org/r/860/diff/2/?file=21213#file21213line310> > > > > 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 Very nice - done. > On 2011-06-17 13:19:13, Alan Conway wrote: > > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp, line 665 > > <https://reviews.apache.org/r/860/diff/2/?file=21214#file21214line665> > > > > static local variables are not thread safe. Use a normal local variable > > constructing an intrusive pointer is trivial. done. > On 2011-06-17 13:19:13, Alan Conway wrote: > > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp, line 804 > > <https://reviews.apache.org/r/860/diff/2/?file=21215#file21215line804> > > > > 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 done. > On 2011-06-17 13:19:13, Alan Conway wrote: > > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp, line 966 > > <https://reviews.apache.org/r/860/diff/2/?file=21215#file21215line966> > > > > See comments above: this can now be > > > > > > async->registerCallback(boost::bind(&AsyncMessageAcceptCmd::dequeueDone, > > this)) done. > On 2011-06-17 13:19:13, Alan Conway wrote: > > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h, line 437 > > <https://reviews.apache.org/r/860/diff/2/?file=21213#file21213line437> > > > > 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? > On 2011-06-17 13:19:13, Alan Conway wrote: > > /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.h, line 107 > > <https://reviews.apache.org/r/860/diff/2/?file=21210#file21210line107> > > > > 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: > > ----------------------------------------------------------- > 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 > >
