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