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