Re: Review Request 12289: QPID-4327: TransactionObserver interface
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12289/ --- (Updated July 12, 2013, 4:19 p.m.) Review request for qpid, Andrew Stitcher and Gordon Sim. Changes --- Added bug link Summary (updated) - QPID-4327: TransactionObserver interface Bugs: QPID-4327 https://issues.apache.org/jira/browse/QPID-4327 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
Re: Review Request 12289: QPID-4327: TransactionObserver interface.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12289/#review22810
---
/trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h
https://reviews.apache.org/r/12289/#comment46520
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_ptrQueue) = 0;
void dequeued(TransactionContext, const Message, const
boost::shared_ptrQueue) = 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.
- Gordon Sim
On July 5, 2013, 9:34 p.m., Alan Conway wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12289/
---
(Updated July 5, 2013, 9:34 p.m.)
Review request for qpid, Andrew Stitcher and Gordon Sim.
Repository: qpid
Description
---
QPID-4327: TransactionObserver interface.
Plugin can set TransactionObserverFactory to create TransactionObservers.
TransactionObserver interface is called at each point in a transactions
lifecycle.
Currently only allows a single TransactionObserverFactory per broker.
This is a bit ugly, any ideas to make it neater would be much appreciated.
Diffs
-
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1500107
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1500107
/trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1500107
/trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h PRE-CREATION
/trunk/qpid/cpp/src/qpid/broker/TxBuffer.h 1500107
/trunk/qpid/cpp/src/tests/brokertest.py 1500107
/trunk/qpid/cpp/src/tests/ha_tests.py 1500107
Diff: https://reviews.apache.org/r/12289/diff/
Testing
---
It compiles
Thanks,
Alan Conway
Re: Review Request 12289: QPID-4327: TransactionObserver interface.
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_ptrQueue) = 0;
void dequeued(TransactionContext, const Message, const
boost::shared_ptrQueue) = 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.
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...
- Alan
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12289/#review22810
---
On July 5, 2013, 9:34 p.m., Alan Conway wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12289/
---
(Updated July 5, 2013, 9:34 p.m.)
Review request for qpid, Andrew Stitcher and Gordon Sim.
Repository: qpid
Description
---
QPID-4327: TransactionObserver interface.
Plugin can set TransactionObserverFactory to create TransactionObservers.
TransactionObserver interface is called at each point in a transactions
lifecycle.
Currently only allows a single TransactionObserverFactory per broker.
This is a bit ugly, any ideas to make it neater would be much appreciated.
Diffs
-
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1500107
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1500107
/trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1500107
/trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h PRE-CREATION
/trunk/qpid/cpp/src/qpid/broker/TxBuffer.h 1500107
/trunk/qpid/cpp/src/tests/brokertest.py 1500107
/trunk/qpid/cpp/src/tests/ha_tests.py 1500107
Diff: https://reviews.apache.org/r/12289/diff/
Testing
---
It compiles
Thanks,
Alan Conway
Re: Review Request 12289: QPID-4327: TransactionObserver interface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12289/ --- (Updated July 8, 2013, 8:40 p.m.) Review request for qpid, Andrew Stitcher and Gordon Sim. Changes --- Implemented call points, added unit tests. Not the final story yet, I will rework based on Gordon's comments https://reviews.apache.org/r/12289/diff/1/?file=318402#file318402line48 Repository: qpid Description (updated) --- QPID-4327: TransactionObserver interface. A plugin can set the Broker's TransactionObserverFactory to create TransactionObservers. The TransactionObserver interface is called at each point in a transactions lifecycle. Currently only allows a single TransactionObserverFactory per broker. Diffs (updated) - /trunk/qpid/cpp/src/qpid/broker/Broker.h 1500613 /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1500613 /trunk/qpid/cpp/src/qpid/broker/DtxAck.h 1500613 /trunk/qpid/cpp/src/qpid/broker/Queue.h 1500613 /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1500613 /trunk/qpid/cpp/src/qpid/broker/RecoveredDequeue.h 1500613 /trunk/qpid/cpp/src/qpid/broker/RecoveredEnqueue.h 1500613 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1500613 /trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/TxAccept.h 1500613 /trunk/qpid/cpp/src/qpid/broker/TxAccept.cpp 1500613 /trunk/qpid/cpp/src/qpid/broker/TxBuffer.h 1500613 /trunk/qpid/cpp/src/qpid/broker/TxBuffer.cpp 1500613 /trunk/qpid/cpp/src/qpid/broker/TxOp.h 1500613 /trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1500613 /trunk/qpid/cpp/src/tests/CMakeLists.txt 1500613 /trunk/qpid/cpp/src/tests/Makefile.am 1500613 /trunk/qpid/cpp/src/tests/TransactionObserverTest.cpp PRE-CREATION /trunk/qpid/cpp/src/tests/TxMocks.h 1500613 /trunk/qpid/cpp/src/tests/brokertest.py 1500613 /trunk/qpid/cpp/src/tests/ha_tests.py 1500613 /trunk/qpid/cpp/src/tests/test_tools.h 1500613 Diff: https://reviews.apache.org/r/12289/diff/ Testing --- It compiles Thanks, Alan Conway
Re: Review Request 12289: QPID-4327: TransactionObserver interface.
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_ptrQueue) = 0;
void dequeued(TransactionContext, const Message, const
boost::shared_ptrQueue) = 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...
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?
- Gordon
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12289/#review22810
---
On July 8, 2013, 8:40 p.m., Alan Conway wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12289/
---
(Updated July 8, 2013, 8:40 p.m.)
Review request for qpid, Andrew Stitcher and Gordon Sim.
Repository: qpid
Description
---
QPID-4327: TransactionObserver interface.
A plugin can set the Broker's TransactionObserverFactory to create
TransactionObservers. The TransactionObserver interface is called at each
point
in a transactions lifecycle. Currently only allows a single
TransactionObserverFactory per broker.
Diffs
-
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1500613
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1500613
/trunk/qpid/cpp/src/qpid/broker/DtxAck.h 1500613
/trunk/qpid/cpp/src/qpid/broker/Queue.h 1500613
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1500613
/trunk/qpid/cpp/src/qpid/broker/RecoveredDequeue.h 1500613
/trunk/qpid/cpp/src/qpid/broker/RecoveredEnqueue.h 1500613
/trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1500613
/trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h PRE-CREATION
/trunk/qpid/cpp/src/qpid/broker/TxAccept.h 1500613
/trunk/qpid/cpp/src/qpid/broker/TxAccept.cpp 1500613
/trunk/qpid/cpp/src/qpid/broker/TxBuffer.h 1500613
/trunk/qpid/cpp/src/qpid/broker/TxBuffer.cpp 1500613
/trunk/qpid/cpp/src/qpid/broker/TxOp.h 1500613
/trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1500613
/trunk/qpid/cpp/src/tests/CMakeLists.txt 1500613
/trunk/qpid/cpp/src/tests/Makefile.am 1500613
/trunk/qpid/cpp/src/tests/TransactionObserverTest.cpp PRE-CREATION
/trunk/qpid/cpp/src/tests/TxMocks.h 1500613
/trunk/qpid/cpp/src/tests/brokertest.py 1500613
/trunk/qpid/cpp/src/tests/ha_tests.py 1500613
/trunk/qpid/cpp/src/tests/test_tools.h 1500613
Diff: https://reviews.apache.org/r/12289/diff/
Testing
---
It compiles
Thanks,
Alan Conway
Review Request 12289: QPID-4327: TransactionObserver interface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12289/ --- Review request for qpid, Andrew Stitcher and Gordon Sim. Repository: qpid Description --- QPID-4327: TransactionObserver interface. Plugin can set TransactionObserverFactory to create TransactionObservers. TransactionObserver interface is called at each point in a transactions lifecycle. Currently only allows a single TransactionObserverFactory per broker. This is a bit ugly, any ideas to make it neater would be much appreciated. Diffs - /trunk/qpid/cpp/src/qpid/broker/Broker.h 1500107 /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1500107 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1500107 /trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/TxBuffer.h 1500107 /trunk/qpid/cpp/src/tests/brokertest.py 1500107 /trunk/qpid/cpp/src/tests/ha_tests.py 1500107 Diff: https://reviews.apache.org/r/12289/diff/ Testing --- It compiles Thanks, Alan Conway
