----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4085/#review5395 -----------------------------------------------------------
branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11719> This is a strange looking sequence diagram, probably better to use a more standard type of sequence diagram branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11723> See below - these are also Handles and so should encapsulate the ref counting not expose it. branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11720> I can see no good reason to make this handle object ref counted - it is a handle object it is supposed to *do* ref counting itself to insulate everyone else from it. branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11721> As previous comment - additionally I'd question the justification for using an intrusive_ptr. The only reason for using an intrusive_ptr in our context is if you need to pass a pointer to an object to some external API and recover the reference counting after getting the raw pointer back. As long as you use make_shared() to create a shared_ptr there is no substantial performance difference. branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11722> Don't do construction/destruction inline like this in API code. Do it a the .cpp branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11731> I think the design here is confused: IMO it only makes sense to create specific Success and Failure objects if you are goinf to pass them to a callback that could take either, as in the suggestion of having a result callback attached to every operation. If you are going to have interface wide callbacks as in this design you don't need the extra objects. Also as specified there is no way to tie operations with results only the things that were operated on - this will lead to ambiguous results if there are multiple operations for a single handle in flight. branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11729> I think the parameter here should be the composite HandlePair with store and broker context not just the brokerContext. Possible you don't even need this object at all - just use the appropriate Handle that was passed into the Operation. branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11730> Probably the same here - don't really need the extra object just make the submit failure signature bigger to take the extras params. branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11726> This comment does no correspond to the interface below. branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11727> I don't like replacing the individual operation callbacks with the pair of fixed callbacks for the entire interface. It is possible to get to the 2 callbacks from a callback per operation but not possible the other way round. branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11724> I don't understand what "next...()" means in the name of the Handle factory methods. The interface is creating Handles so create...Handle(...) or make...Handle() would be more conventional. branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h <https://reviews.apache.org/r/4085/#comment11725> Don't use abbreviations like Ctxt or Hndl. brokerContext or dataHandle is much better documentation if you don't speak English well. - Andrew On 2012-02-28 16:29:47, Kim van der Riet wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4085/ > ----------------------------------------------------------- > > (Updated 2012-02-28 16:29:47) > > > Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, Kenneth > Giusti, and Ted Ross. > > > Summary > ------- > > This is the second design draft for an asyncronous store interface after > feedback from the previous review was received. (See QPID-3858 > https://issues.apache.org/jira/browse/QPID-3858 for an overview.) > > Much of the commentary is in the source. Comments are welcome. > > > Diffs > ----- > > branches/asyncstore/cpp/src/CMakeLists.txt 1294116 > branches/asyncstore/cpp/src/Makefile.am 1294116 > branches/asyncstore/cpp/src/qpid/broker/AsyncInterface.h PRE-CREATION > > Diff: https://reviews.apache.org/r/4085/diff > > > Testing > ------- > > > Thanks, > > Kim > >
