You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@qpid.apache.org by Kim van der Riet <ki...@redhat.com> on 2012/03/08 15:09:21 UTC

Review Request: Proposed Asynchronous Store Interface for C++ broker

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/
-----------------------------------------------------------

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.

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

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Kim van der Riet <ki...@redhat.com>.


> On 2012-03-09 14:59:32, Gordon Sim wrote:
> > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h, line 77
> > <https://reviews.apache.org/r/4243/diff/3/?file=89131#file89131line77>
> >
> >     I don't like this form of the EnqueueHandle factory (and I don't like the name of the handle either).
> >     
> >     For one thing you can't have the TxnHandle passed in here and not in the submitEnqueue/Dequeue as you can never dequeue under the same transaction as you enqueue.

If you prefer the old form of the Handle Factory, then your suggested calls for the factory and ops:

MessageHandle createMessageHandle(DataSource*);
EnqueuedMessageHandle createEnqueuedMessageHandle(MessageHandle&, Queuehandle&);

void submitEnqueue(EnqueuedMessageHandle&, TxnHandle&, ResultCallback&, BrokerContext&);
void submitDequeue(EnqueuedMessageHandle&, TxnHandle&, ResultCallback&, BrokerContext&);

should be modified to match the existing patterns. For example:

MessageHandle createMessageHandle(DataSource*);
EnqueuedMessageHandle createEnqueuedMessageHandle();

void submitEnqueue(EnqueuedMessageHandle&, MessageHandle&, Queuehandle&, TxnHandle&, ResultCallback&, BrokerContext&);
void submitDequeue(EnqueuedMessageHandle&, TxnHandle&, ResultCallback&, BrokerContext&);

On transactions, you are quite right - I have overlooked that point. The Transaction Token will have to be supplied to the operation directly, no matter which version of the Handle Factory we use.


> On 2012-03-09 14:59:32, Gordon Sim wrote:
> > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h, line 72
> > <https://reviews.apache.org/r/4243/diff/3/?file=89131#file89131line72>
> >
> >     I don't see the need for DataHandle.

You suggested it, but it was called MessageHandle in your proposal (see below).

This allows the store to add some kind of identification to the source. If the store wants to optimize the storage of data when it sees the same data source in different operations (such as storing the same message on more than one queue), then the store can use this identification to recognize the data. Without this handle, I don't see how the store can do this.

Of course, this is optional. If the store does not want to optimize this way (for example, the current async store), then it can simply return the DataSource object as the handle (or an encapsulated version of it).


- Kim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review5780
-----------------------------------------------------------


On 2012-03-08 14:17:57, Kim van der Riet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4243/
> -----------------------------------------------------------
> 
> (Updated 2012-03-08 14:17:57)
> 
> 
> 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
> 
>

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Gordon Sim <gs...@redhat.com>.


> On 2012-03-09 14:59:32, Gordon Sim wrote:
> > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h, line 72
> > <https://reviews.apache.org/r/4243/diff/3/?file=89131#file89131line72>
> >
> >     I don't see the need for DataHandle.
> 
> Kim van der Riet wrote:
>     You suggested it, but it was called MessageHandle in your proposal (see below).
>     
>     This allows the store to add some kind of identification to the source. If the store wants to optimize the storage of data when it sees the same data source in different operations (such as storing the same message on more than one queue), then the store can use this identification to recognize the data. Without this handle, I don't see how the store can do this.
>     
>     Of course, this is optional. If the store does not want to optimize this way (for example, the current async store), then it can simply return the DataSource object as the handle (or an encapsulated version of it).
>

I suggested something different - a MessageHandle that would be shared across enqueues that had the same content. That is not the same as a generic DataHandle as used here and passed to all operations that need a DataSource.

I'm actually still not entirely convinced having the 'same message' for multiple enqueues is even needed, though I could probably live with it. Does the windows based store do any optimisation for that case I wonder...


> On 2012-03-09 14:59:32, Gordon Sim wrote:
> > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h, line 77
> > <https://reviews.apache.org/r/4243/diff/3/?file=89131#file89131line77>
> >
> >     I don't like this form of the EnqueueHandle factory (and I don't like the name of the handle either).
> >     
> >     For one thing you can't have the TxnHandle passed in here and not in the submitEnqueue/Dequeue as you can never dequeue under the same transaction as you enqueue.
> 
> Kim van der Riet wrote:
>     If you prefer the old form of the Handle Factory, then your suggested calls for the factory and ops:
>     
>     MessageHandle createMessageHandle(DataSource*);
>     EnqueuedMessageHandle createEnqueuedMessageHandle(MessageHandle&, Queuehandle&);
>     
>     void submitEnqueue(EnqueuedMessageHandle&, TxnHandle&, ResultCallback&, BrokerContext&);
>     void submitDequeue(EnqueuedMessageHandle&, TxnHandle&, ResultCallback&, BrokerContext&);
>     
>     should be modified to match the existing patterns. For example:
>     
>     MessageHandle createMessageHandle(DataSource*);
>     EnqueuedMessageHandle createEnqueuedMessageHandle();
>     
>     void submitEnqueue(EnqueuedMessageHandle&, MessageHandle&, Queuehandle&, TxnHandle&, ResultCallback&, BrokerContext&);
>     void submitDequeue(EnqueuedMessageHandle&, TxnHandle&, ResultCallback&, BrokerContext&);
>     
>     On transactions, you are quite right - I have overlooked that point. The Transaction Token will have to be supplied to the operation directly, no matter which version of the Handle Factory we use.

I think to be able to properly exploit the single message-multiple enqueues, its better to have the MessageHandle and QueueHandle passed to the createEnqueueMessageHandle() factory. That way that factory can decide what type to return based on the information supplied (which is the whole point of this factory methods).


- Gordon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review5780
-----------------------------------------------------------


On 2012-03-08 14:17:57, Kim van der Riet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4243/
> -----------------------------------------------------------
> 
> (Updated 2012-03-08 14:17:57)
> 
> 
> 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
> 
>

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Gordon Sim <gs...@redhat.com>.


> On 2012-03-09 14:59:32, Gordon Sim wrote:
> > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h, line 72
> > <https://reviews.apache.org/r/4243/diff/3/?file=89131#file89131line72>
> >
> >     I don't see the need for DataHandle.
> 
> Kim van der Riet wrote:
>     You suggested it, but it was called MessageHandle in your proposal (see below).
>     
>     This allows the store to add some kind of identification to the source. If the store wants to optimize the storage of data when it sees the same data source in different operations (such as storing the same message on more than one queue), then the store can use this identification to recognize the data. Without this handle, I don't see how the store can do this.
>     
>     Of course, this is optional. If the store does not want to optimize this way (for example, the current async store), then it can simply return the DataSource object as the handle (or an encapsulated version of it).
>
> 
> Gordon Sim wrote:
>     I suggested something different - a MessageHandle that would be shared across enqueues that had the same content. That is not the same as a generic DataHandle as used here and passed to all operations that need a DataSource.
>     
>     I'm actually still not entirely convinced having the 'same message' for multiple enqueues is even needed, though I could probably live with it. Does the windows based store do any optimisation for that case I wonder...
> 
> Kim van der Riet wrote:
>     You are correct, but I was also thinking of the new Event storage. Because an event is also stored on a per-queue basis, it might be possible that a single event data source could be saved on more than one queue. This expands the idea beyond just that of a message, and thus the DataHandle idea was born. I agree that the remaining methods that require a data source don't fall into this category, and it would be simpler (and more transparent) to leave the interface as DataSource*.

I don't see any good use case for having the same event recorded for more than one queue. All the use cases I have in mind would be queue specific. In any case they are also likely to be small in terms of data footprint, meaning that trying to optimise for common content is even less important.


- Gordon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review5780
-----------------------------------------------------------


On 2012-03-08 14:17:57, Kim van der Riet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4243/
> -----------------------------------------------------------
> 
> (Updated 2012-03-08 14:17:57)
> 
> 
> 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
> 
>

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Kim van der Riet <ki...@redhat.com>.


> On 2012-03-09 14:59:32, Gordon Sim wrote:
> > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h, line 72
> > <https://reviews.apache.org/r/4243/diff/3/?file=89131#file89131line72>
> >
> >     I don't see the need for DataHandle.
> 
> Kim van der Riet wrote:
>     You suggested it, but it was called MessageHandle in your proposal (see below).
>     
>     This allows the store to add some kind of identification to the source. If the store wants to optimize the storage of data when it sees the same data source in different operations (such as storing the same message on more than one queue), then the store can use this identification to recognize the data. Without this handle, I don't see how the store can do this.
>     
>     Of course, this is optional. If the store does not want to optimize this way (for example, the current async store), then it can simply return the DataSource object as the handle (or an encapsulated version of it).
>
> 
> Gordon Sim wrote:
>     I suggested something different - a MessageHandle that would be shared across enqueues that had the same content. That is not the same as a generic DataHandle as used here and passed to all operations that need a DataSource.
>     
>     I'm actually still not entirely convinced having the 'same message' for multiple enqueues is even needed, though I could probably live with it. Does the windows based store do any optimisation for that case I wonder...

You are correct, but I was also thinking of the new Event storage. Because an event is also stored on a per-queue basis, it might be possible that a single event data source could be saved on more than one queue. This expands the idea beyond just that of a message, and thus the DataHandle idea was born. I agree that the remaining methods that require a data source don't fall into this category, and it would be simpler (and more transparent) to leave the interface as DataSource*.


> On 2012-03-09 14:59:32, Gordon Sim wrote:
> > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h, line 77
> > <https://reviews.apache.org/r/4243/diff/3/?file=89131#file89131line77>
> >
> >     I don't like this form of the EnqueueHandle factory (and I don't like the name of the handle either).
> >     
> >     For one thing you can't have the TxnHandle passed in here and not in the submitEnqueue/Dequeue as you can never dequeue under the same transaction as you enqueue.
> 
> Kim van der Riet wrote:
>     If you prefer the old form of the Handle Factory, then your suggested calls for the factory and ops:
>     
>     MessageHandle createMessageHandle(DataSource*);
>     EnqueuedMessageHandle createEnqueuedMessageHandle(MessageHandle&, Queuehandle&);
>     
>     void submitEnqueue(EnqueuedMessageHandle&, TxnHandle&, ResultCallback&, BrokerContext&);
>     void submitDequeue(EnqueuedMessageHandle&, TxnHandle&, ResultCallback&, BrokerContext&);
>     
>     should be modified to match the existing patterns. For example:
>     
>     MessageHandle createMessageHandle(DataSource*);
>     EnqueuedMessageHandle createEnqueuedMessageHandle();
>     
>     void submitEnqueue(EnqueuedMessageHandle&, MessageHandle&, Queuehandle&, TxnHandle&, ResultCallback&, BrokerContext&);
>     void submitDequeue(EnqueuedMessageHandle&, TxnHandle&, ResultCallback&, BrokerContext&);
>     
>     On transactions, you are quite right - I have overlooked that point. The Transaction Token will have to be supplied to the operation directly, no matter which version of the Handle Factory we use.
> 
> Gordon Sim wrote:
>     I think to be able to properly exploit the single message-multiple enqueues, its better to have the MessageHandle and QueueHandle passed to the createEnqueueMessageHandle() factory. That way that factory can decide what type to return based on the information supplied (which is the whole point of this factory methods).

Yes, that is a very good point.


- Kim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review5780
-----------------------------------------------------------


On 2012-03-08 14:17:57, Kim van der Riet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4243/
> -----------------------------------------------------------
> 
> (Updated 2012-03-08 14:17:57)
> 
> 
> 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
> 
>

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Gordon Sim <gs...@redhat.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review5780
-----------------------------------------------------------



branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h
<https://reviews.apache.org/r/4243/#comment12595>

    I don't see the need for DataHandle.



branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h
<https://reviews.apache.org/r/4243/#comment12596>

    I don't like this form of the EnqueueHandle factory (and I don't like the name of the handle either).
    
    For one thing you can't have the TxnHandle passed in here and not in the submitEnqueue/Dequeue as you can never dequeue under the same transaction as you enqueue.


- Gordon


On 2012-03-08 14:17:57, Kim van der Riet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4243/
> -----------------------------------------------------------
> 
> (Updated 2012-03-08 14:17:57)
> 
> 
> 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
> 
>

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Alan Conway <ac...@redhat.com>.


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

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Kim van der Riet <ki...@redhat.com>.


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

Yes, that is the idea here (see the comment at the forward declaration).


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

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.


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

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.


- Kim


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

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Alan Conway <ac...@redhat.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review6119
-----------------------------------------------------------



branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h
<https://reviews.apache.org/r/4243/#comment13149>

    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.
    
    



branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h
<https://reviews.apache.org/r/4243/#comment13146>

    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.



branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h
<https://reviews.apache.org/r/4243/#comment13148>

    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.


- Alan


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

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Gordon Sim <gs...@redhat.com>.


> On 2012-03-09 18:38:19, Gordon Sim wrote:
> > In my view this is good enough to get started with. We may have some changes along the way based on findings from implementing above or below the line. I'm still unsure as to whether its worth accommodating the multi-enqueue message for example, but that can be revisted.
> 
> Kim van der Riet wrote:
>     Agreed. We also need to think through how events will be recovered. Enqueued messages are logically dequeued, on recovery only the remaining enqueued records are recovered and passed back to the broker. However, as events have no logical destroy mechanism, they will _all_ have to be recoverd and returned to the broker (or a queue in the broker), and that could amount to a lot of pushing of obsolete status records.
>     
>     Perhaps the right idea is to introduce some sort of optional key, and when this is supplied to a submitRecord() (previously submitStoreEvent()), it behaves logically like an LVQ, it logically replaces any previous record using the same key. The store would have to be aware of the key to be able to analyze and eliminate the older records on recovery though.

Yes, the whole events thing needs a bit more thought. Even more of a concern is the fact that if event records aren't invalidated, you will either run out of journal space or have to lose the records. Your key idea is plausible, but we need to also consider what the use cases might actually be so we don't design something that isn't as useful as it could be.

The two cases I have in mind would be (a) more precise tracking of delivery state for a message and (b) tracking durable consumers 'positions' in a queue.


- Gordon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review5792
-----------------------------------------------------------


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

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Kim van der Riet <ki...@redhat.com>.


> On 2012-03-09 18:38:19, Gordon Sim wrote:
> > In my view this is good enough to get started with. We may have some changes along the way based on findings from implementing above or below the line. I'm still unsure as to whether its worth accommodating the multi-enqueue message for example, but that can be revisted.

Agreed. We also need to think through how events will be recovered. Enqueued messages are logically dequeued, on recovery only the remaining enqueued records are recovered and passed back to the broker. However, as events have no logical destroy mechanism, they will _all_ have to be recoverd and returned to the broker (or a queue in the broker), and that could amount to a lot of pushing of obsolete status records.

Perhaps the right idea is to introduce some sort of optional key, and when this is supplied to a submitRecord() (previously submitStoreEvent()), it behaves logically like an LVQ, it logically replaces any previous record using the same key. The store would have to be aware of the key to be able to analyze and eliminate the older records on recovery though.


> On 2012-03-09 18:38:19, Gordon Sim wrote:
> > branches/asyncstore/cpp/src/qpid/broker/AsyncStore.cpp, line 1
> > <https://reviews.apache.org/r/4243/diff/4/?file=90822#file90822line1>
> >
> >     We probably don't have a need for AsyncStore.cpp, right?

Quite likely we won't. It allows the header to be compiled in the interim, but I imagine it will go away eventually.


- Kim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review5792
-----------------------------------------------------------


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

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Gordon Sim <gs...@redhat.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review5792
-----------------------------------------------------------


In my view this is good enough to get started with. We may have some changes along the way based on findings from implementing above or below the line. I'm still unsure as to whether its worth accommodating the multi-enqueue message for example, but that can be revisted.


branches/asyncstore/cpp/src/qpid/broker/AsyncStore.cpp
<https://reviews.apache.org/r/4243/#comment12629>

    We probably don't have a need for AsyncStore.cpp, right?


- Gordon


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

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Steve Huston <sh...@riverace.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review5804
-----------------------------------------------------------



branches/asyncstore/cpp/src/qpid/broker/AsyncStore.h
<https://reviews.apache.org/r/4243/#comment12672>

    I would really like to see some explanations here about the big picture and how you see this working with the broker. It's hard to dive into this level of detail without that explanation/overview.


- Steve


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

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Kim van der Riet <ki...@redhat.com>.

(Updated 2012-03-09 18:33:10.792180)

Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, Ted Ross, and Steve Huston.

Changes
-------

Updated interface after discussion with Gordon.

Summary
-------

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:

Comments welcome.

Coordinating JIRA: https://issues.apache.org/jira/browse/QPID-3858

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/4243/diff

Testing
-------

Compiles as written, but is not implemented.

Thanks,

Kim

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Kim van der Riet <ki...@redhat.com>.


> On 2012-03-08 17:20:22, Alan Conway wrote:
> > We already have an implementation of opaque, reference-counting handles - see qpid/messaging/Handle.h. I suggest we re-use that, or copy it if we don't want messaging dependencies in a broker-side interface.

That is exactly what I had in mind!


- Kim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review5732
-----------------------------------------------------------


On 2012-03-08 14:17:57, Kim van der Riet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4243/
> -----------------------------------------------------------
> 
> (Updated 2012-03-08 14:17:57)
> 
> 
> 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
> 
>

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Alan Conway <ac...@redhat.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4243/#review5732
-----------------------------------------------------------


We already have an implementation of opaque, reference-counting handles - see qpid/messaging/Handle.h. I suggest we re-use that, or copy it if we don't want messaging dependencies in a broker-side interface.

- Alan


On 2012-03-08 14:17:57, Kim van der Riet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4243/
> -----------------------------------------------------------
> 
> (Updated 2012-03-08 14:17:57)
> 
> 
> 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
> 
>

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Kim van der Riet <ki...@redhat.com>.

(Updated 2012-03-08 14:17:57.779505)

Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, Ted Ross, and Steve Huston.

Changes
-------

Changed class name to match file name - "class AsyncStore"

Summary
-------

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:

Comments welcome.

Coordinating JIRA: https://issues.apache.org/jira/browse/QPID-3858

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/4243/diff

Testing
-------

Compiles as written, but is not implemented.

Thanks,

Kim

Re: Review Request: Proposed Asynchronous Store Interface for C++ broker

Posted by Kim van der Riet <ki...@redhat.com>.

(Updated 2012-03-08 14:15:23.199271)

Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, Ted Ross, and Steve Huston.

Changes
-------

Added JIRA reference

Summary (updated)
-------

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:

Comments welcome.

Coordinating JIRA: https://issues.apache.org/jira/browse/QPID-3858

Diffs
-----

Diff: https://reviews.apache.org/r/4243/diff

Testing
-------

Compiles as written, but is not implemented.

Thanks,

Kim