qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Conway" <acon...@redhat.com>
Subject Re: Review Request: Proposed Asynchronous Store Interface for C++ broker
Date Thu, 22 Mar 2012 13:42:34 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message