> On 2012-03-20 15:20:42, Alan Conway wrote: > > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h, line 63 > > <https://reviews.apache.org/r/4243/diff/4/?file=90821#file90821line63> > > > > There's not enough context here for the callback to determine what > > request this result is for. > > > > Suggest making result callback a functor object and pass it by pointer. > > > > class ResultCallback { > > virtual void operator()(const AsyncResult&, BrokerContext*) = 0; > > } > > > > and pass it by pointer to the sumbit*() functions. That allows the user > > to create an individual result object for each submit call with whatever > > extra. > > The lifecycle is clear: the ResultCallback can be deleted in or after > > it's operator(). If a user doesn't need per-operation data they can just > > create a single callback instance and pass it to every submit. > > > > > > Kim van der Riet wrote: > I had thought that the instance of BrokerContext would supply the > necessary context, containing whatever is needed for the broker to > appropriately handle the callback. This object is opaque to the store, and is > simply passed back to the broker in the callback.
OK, I was misled by the name. BrokerContext implies the context is associated with the broker, but it's associated with a single async request. Suggest AsyncRequestContext or somesuch. > On 2012-03-20 15:20:42, Alan Conway wrote: > > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h, line 83 > > <https://reviews.apache.org/r/4243/diff/4/?file=90821#file90821line83> > > > > What's the scope of BrokerContext, and how do we ensure it is not > > deleted while still being used? It may need to be refcounted. > > Kim van der Riet wrote: > Yes, that is the idea here (see the comment at the forward declaration). Again, perhaps a virtual destroy() method would be better than refcounting. That lets the caller clean up however they like. > On 2012-03-20 15:20:42, Alan Conway wrote: > > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h, line 77 > > <https://reviews.apache.org/r/4243/diff/4/?file=90821#file90821line77> > > > > What are the call semantics for DataSource? Is it called synchronously > > in createMessageHandle or called asynchronsouly in another thread? If the > > latter then DataStore needs to be ref-counted. It's worth a comment in the > > header file. Same thing applies to all the later instances of DataStore* > > parameters. > > Kim van der Riet wrote: > The latter of these - it supplies a callback for another worker thread to > asynchronously make a callback on the write() method. In the case of the > store, it would call write(addr) with addr pointing to the correct location > in the page buffers, and the source (the message object in the case of an > enqueue op) would perform the write. > > This does presuppose that the broker (or whomever supplies the write() > callback) would keep the data to be written stable and available (effectively > locked?) until the appropriate completion has been received from the store. > The completion could then delete the DataSource object, although this does > not preclude using a RefCounted solution. Perhaps a virtual destroy() method would be better than refcounting. That lets the caller clean up however they like (which could be via refcounting or something else) - Alan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4243/#review6119 ----------------------------------------------------------- On 2012-03-09 18:33:10, Kim van der Riet wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4243/ > ----------------------------------------------------------- > > (Updated 2012-03-09 18:33:10) > > > Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, Ted Ross, > and Steve Huston. > > > Summary > ------- > > The async store interface is intended to replace the current syncronous store > interface in broker/MessageStore.h. It presents a bidirectional async > interface, allowing async store operations which contain optional callbacks > to be queued up for processing by the store, and for the results of these > operations (if the callback is present in the operation) to be queued up for > action by the broker itself. > > After a good deal of back-and-forth on the list, this is the latest iteration > on the async store interface. Thanks to Andrew and Gordon who have provided > much valuable feedback. > > If this interface is accepted, it will need to be implemented in two phases: > > 1. Broker wiring - the broker will need to be hooked into this interface > using async semantics. This could be a disrptive phase, as it would likely > break or disconnect the current sync interface. An async-sync adaptor would > also be developed to allow current syncronous stores to continue operating > under the new interface. > > 2. Async store development - the current async store is not currently part of > the Qpid codebase as it was developed under an incompatible license (LGPL2). > A new async store would be developed under Apache, with any bits moved from > the old code base dual-licenced to Apache. (This will require some legal > checkups.) > > Comments welcome. > > Coordinating JIRA: https://issues.apache.org/jira/browse/QPID-3858 > > > Diffs > ----- > > branches/asyncstore/cpp/src/CMakeLists.txt 1291264 > branches/asyncstore/cpp/src/Makefile.am 1291264 > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h PRE-CREATION > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/4243/diff > > > Testing > ------- > > Compiles as written, but is not implemented. > > > Thanks, > > Kim > >
