> On July 8, 2013, 12:10 p.m., Gordon Sim wrote: > > /trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h, line 48 > > <https://reviews.apache.org/r/12289/diff/1/?file=318402#file318402line48> > > > > It's really the enqueue and dequeue operations that are transactional. > > It might be better to define this interface in terms of those. > > > > It would also seem - on the surface at least - a little more consistent > > to have a set of TransactionObservers at the broker level (along side those > > for queues and connections), rather than a factory that is then used to > > attach different observer instances to transaction buffers. > > > > E.g. > > > > class TransactionObserver { > > void enqueued(TransactionContext&, const Message&, const > > boost::shared_ptr<Queue>) = 0; > > void dequeued(TransactionContext&, const Message&, const > > boost::shared_ptr<Queue>) = 0; > > void started(TransactionContext&) = 0; > > void prepared(TransactionContext&) = 0; > > void committed(TransactionContext&) = 0; > > void rolledback(TransactionContext&) = 0; > > } > > > > Perhaps adding a 'std::string getId()' method to TransactionContext if > > needed. > > > > Though 1.0 doesn't yet support transactions, DeliveryRecords is an 0-10 > > specific class so the interface you have here would need to change for 1.0 > > anyway I suspect. > > Alan Conway wrote: > Regarding "It would also seem - on the surface at least - a little more > consistent to have a set of TransactionObservers at the broker level (along > side those for queues and connections), rather than a factory that is then > used to attach different observer instances to transaction buffers." > > The way I have it is more consistent with e.g. QueueObserver - an > individual observer for each queue. It avoids another layer of maps by going > direct to the correct observer. There is no QueueObserverFactory because a > plugin can register a ConfigObserver and be informed of queue creation, so > can attach it's observer. There isn't such an intercept point for > transactions (that I know of) so I added the factory. Maybe there's a more > consistent way to add such a point - ConfigObserver doesn't seem the right > place though. Maybe ConfigObserver could be renamed to something more generic > (BrokerEventObserver? yuck.) > > Regarding your interface above I'll rework things on those lines... > > Gordon Sim wrote: > What about having a broker level set of observers, but adding those to > each TransactionContext/TransactionBuffer and allowing code to attach > additional observers to specific TransactionContexts/TransactionBuffers if > desired?
I think the changes in diff 4 address these issues: - TranactionObserver doesn't use DeliveryRecords, it uses queue position and replication SequenceNumbers to ID a message. - ConfigObserver has been renamed BrokerObserver and provides startTx and startDtx methods to observe creation of a transaction. - Plugins that wish to can add a TransactionObserver to the transaction to see accept, publish, prepare, commit and rollback. - Alan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12289/#review22810 ----------------------------------------------------------- On July 10, 2013, 10:28 p.m., Alan Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12289/ > ----------------------------------------------------------- > > (Updated July 10, 2013, 10:28 p.m.) > > > Review request for qpid, Andrew Stitcher and Gordon Sim. > > > Repository: qpid > > > Description > ------- > > QPID-4327: HA added PrimaryTransactionObserver. > > Initial implementation only logs some trace messages. > > QPID-4327: TransactionObserver interface. > > Added TransactionObserver interface, called at each point in a transaction's > lifecycle. Currently only a single observer can be associated with a > transaction. > > Added startTx, startDtx to BrokerObserver so plugins can observe transactions > starting and set a TransactionObserver. > > QPID-4327: Renamed ConfigurationObserver as BrokerObserver. > > This class really was intended as a observer for broker-level events which > includes configuration but may in future include other non-configuration > events > such as transactions. > > QPID-4327: Refactor to simplify TxAccept. > > Removed un-necessary RangeOps layers. > > > Diffs > ----- > > /trunk/qpid/cpp/src/CMakeLists.txt 1501768 > /trunk/qpid/cpp/src/Makefile.am 1501768 > /trunk/qpid/cpp/src/ha.mk 1501768 > /trunk/qpid/cpp/src/qpid/broker/Broker.h 1501768 > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1501768 > /trunk/qpid/cpp/src/qpid/broker/BrokerObserver.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/broker/BrokerObservers.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/broker/ConfigurationObserver.h 1501768 > /trunk/qpid/cpp/src/qpid/broker/ConfigurationObservers.h 1501768 > /trunk/qpid/cpp/src/qpid/broker/DtxAck.h 1501768 > /trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp 1501768 > /trunk/qpid/cpp/src/qpid/broker/Queue.h 1501768 > /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1501768 > /trunk/qpid/cpp/src/qpid/broker/QueueRegistry.cpp 1501768 > /trunk/qpid/cpp/src/qpid/broker/RecoveredDequeue.h 1501768 > /trunk/qpid/cpp/src/qpid/broker/RecoveredEnqueue.h 1501768 > /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1501768 > /trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/broker/TxAccept.h 1501768 > /trunk/qpid/cpp/src/qpid/broker/TxAccept.cpp 1501768 > /trunk/qpid/cpp/src/qpid/broker/TxBuffer.h 1501768 > /trunk/qpid/cpp/src/qpid/broker/TxBuffer.cpp 1501768 > /trunk/qpid/cpp/src/qpid/broker/TxOp.h 1501768 > /trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1501768 > /trunk/qpid/cpp/src/qpid/ha/Primary.h 1501768 > /trunk/qpid/cpp/src/qpid/ha/Primary.cpp 1501768 > /trunk/qpid/cpp/src/qpid/ha/PrimaryTransactionObserver.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/ha/PrimaryTransactionObserver.cpp PRE-CREATION > /trunk/qpid/cpp/src/qpid/ha/QueueSnapshots.h 1501768 > /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.h 1501768 > /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.cpp 1501768 > /trunk/qpid/cpp/src/tests/CMakeLists.txt 1501768 > /trunk/qpid/cpp/src/tests/Makefile.am 1501768 > /trunk/qpid/cpp/src/tests/TransactionObserverTest.cpp PRE-CREATION > /trunk/qpid/cpp/src/tests/TxMocks.h 1501768 > /trunk/qpid/cpp/src/tests/brokertest.py 1501768 > /trunk/qpid/cpp/src/tests/ha_tests.py 1501768 > /trunk/qpid/cpp/src/tests/test_env.sh.in 1501768 > /trunk/qpid/cpp/src/tests/test_tools.h 1501768 > > Diff: https://reviews.apache.org/r/12289/diff/ > > > Testing > ------- > > It compiles > > > Thanks, > > Alan Conway > >
