You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2013/07/05 23:34:43 UTC

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


Re: Review Request 12289: QPID-4327: HA TX transactions: basic replication.

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12289/
-----------------------------------------------------------

(Updated July 31, 2013, 1:34 p.m.)


Review request for qpid, Andrew Stitcher and Gordon Sim.


Changes
-------

Closer to final code:
- added dummy store to verify actions on store.
- de-duplication of transactional messages

Unfinished:
- Primary does not wait for backups to prepare() before committing.
- All connected backups are assumed to be in the transaction, there are race
  conditions around brokers joining/leavinv where this assumption is invalid.
- Need more tests.


Summary (updated)
-----------------

QPID-4327: HA TX transactions: basic replication.


Bugs: QPID-4327
    https://issues.apache.org/jira/browse/QPID-4327


Repository: qpid


Description (updated)
-------

QPID-4327: HA TX transactions: basic replication.

On primary a PrimaryTxObserver observes a transaction's TxBuffer and generates
transaction events on a tx-replication-queue. On the backup a TxReplicator
receives the events and constructs a TxBuffer equivalent to the one in the
primary.

Unfinished:
- Primary does not wait for backups to prepare() before committing.
- All connected backups are assumed to be in the transaction, there are race
  conditions around brokers joining/leavinv where this assumption is invalid.
- Need more tests.

QPID-4327: HA get rid of Primary::get() singleton.


QPID-4327: Added 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.

QPID-4327: Optimize brokertest.ready() to improve test runtimes.


Diffs (updated)
-----

  /trunk/qpid/cpp/include/qpid/framing/BufferTypes.h PRE-CREATION 
  /trunk/qpid/cpp/src/CMakeLists.txt 1508523 
  /trunk/qpid/cpp/src/qpid/broker/Broker.h 1508523 
  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1508523 
  /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 1508523 
  /trunk/qpid/cpp/src/qpid/broker/ConfigurationObservers.h 1508523 
  /trunk/qpid/cpp/src/qpid/broker/DtxAck.h 1508523 
  /trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/broker/MessageStoreModule.h 1508523 
  /trunk/qpid/cpp/src/qpid/broker/MessageStoreModule.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/broker/Queue.h 1508523 
  /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/broker/QueueRegistry.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/broker/RecoveredDequeue.h 1508523 
  /trunk/qpid/cpp/src/qpid/broker/RecoveredEnqueue.h 1508523 
  /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/broker/TxAccept.h 1508523 
  /trunk/qpid/cpp/src/qpid/broker/TxAccept.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/broker/TxBuffer.h 1508523 
  /trunk/qpid/cpp/src/qpid/broker/TxBuffer.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/broker/TxOp.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/Backup.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/ha/Event.h PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/Event.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/FailoverExchange.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/ha/HaBroker.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/ha/Membership.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/Membership.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/ha/Primary.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/Primary.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/ha/PrimaryTxObserver.h PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/PrimaryTxObserver.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/ha/QueueSnapshots.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/ha/StatusCheck.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/ha/TxReplicator.h PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/TxReplicator.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/makeMessage.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/makeMessage.cpp 1508523 
  /trunk/qpid/cpp/src/qpid/ha/types.h 1508523 
  /trunk/qpid/cpp/src/qpid/ha/types.cpp 1508523 
  /trunk/qpid/cpp/src/tests/BrokerFixture.h 1508523 
  /trunk/qpid/cpp/src/tests/CMakeLists.txt 1508523 
  /trunk/qpid/cpp/src/tests/MessagingFixture.h 1508523 
  /trunk/qpid/cpp/src/tests/TransactionObserverTest.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/tests/TxMocks.h 1508523 
  /trunk/qpid/cpp/src/tests/brokertest.py 1508523 
  /trunk/qpid/cpp/src/tests/cluster_test.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/tests/ha_test.py 1508523 
  /trunk/qpid/cpp/src/tests/ha_tests.py 1508523 
  /trunk/qpid/cpp/src/tests/test_env.sh.in 1508523 
  /trunk/qpid/cpp/src/tests/test_store.cpp 1508523 
  /trunk/qpid/cpp/src/tests/test_tools.h 1508523 

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


Testing
-------

Basic tx replication appears to be working, used temporary log statements to prove the messages were being processed by the TxBuffer on the backup and not just being sent by the normal means.


Thanks,

Alan Conway


Re: Review Request 12289: QPID-4327: HA TxReplicator, basic replication of transactions.

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12289/
-----------------------------------------------------------

(Updated July 24, 2013, 7:51 p.m.)


Review request for qpid, Andrew Stitcher and Gordon Sim.


Summary (updated)
-----------------

QPID-4327: HA TxReplicator, basic replication of transactions.


Bugs: QPID-4327
    https://issues.apache.org/jira/browse/QPID-4327


Repository: qpid


Description (updated)
-------

Basic tx replication is working but there are a bunch of questions marked in the code as to whether we're doing things correctly or not.

TxBuffer on the primary calls PrimaryTxObserver with tx events. The observer puts events onto a special tx-queue. The backup TxReplicator is QueueReplicator subclass that constructs a corresponding TxBuffer on the backup. Handling dequeues is complicated by the way we collect acks on the session up to prepare/commit and then process them. TxReplicator does something similar but this is the most dubious area of the code.

There are tests in ha_tests.py but the dequeue tests also pass due to normal queue replication, so hard to verify the tx stuff is really working.
Used temporary log statements to prove it was but need a better test. Ideas?

Here is the commit log:

QPID-4327: HA TxReplicator, basic replication of transactions.

TxReplicator is a specialized QueueReplicator to hand tx-queues.
- PrimaryTxObserver puts transaction events on the tx-queue.
- TxReplicator creates a replica TxBuffer on the backup.

QPID-4327: Add PrimaryTransactionObserver and tests.

- PrimaryTransactionObserver observes tx evens and populates tx-queue
- ClusterFixture for unit tests, manages ports for brokers.

QPID-4327: Refactor events & dispatching

- added ha/Event.h for encoding event messages
- hashmap dispatching for QueueReplicator

QPID-4327: Added 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.

QPID-4327: Optimize brokertest.ready() to improve test runtimes.


Diffs (updated)
-----

  /trunk/qpid/cpp/include/qpid/framing/Buffer.h 1506613 
  /trunk/qpid/cpp/include/qpid/framing/BufferTypes.h PRE-CREATION 
  /trunk/qpid/cpp/src/CMakeLists.txt 1506613 
  /trunk/qpid/cpp/src/Makefile.am 1506613 
  /trunk/qpid/cpp/src/ha.mk 1506613 
  /trunk/qpid/cpp/src/qpid/broker/Broker.h 1506613 
  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1506613 
  /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 1506613 
  /trunk/qpid/cpp/src/qpid/broker/ConfigurationObservers.h 1506613 
  /trunk/qpid/cpp/src/qpid/broker/DtxAck.h 1506613 
  /trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/broker/Queue.h 1506613 
  /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/broker/QueueRegistry.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/broker/RecoveredDequeue.h 1506613 
  /trunk/qpid/cpp/src/qpid/broker/RecoveredEnqueue.h 1506613 
  /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/broker/TxAccept.h 1506613 
  /trunk/qpid/cpp/src/qpid/broker/TxAccept.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/broker/TxBuffer.h 1506613 
  /trunk/qpid/cpp/src/qpid/broker/TxBuffer.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/broker/TxOp.h 1506613 
  /trunk/qpid/cpp/src/qpid/ha/Backup.h 1506613 
  /trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.h 1506613 
  /trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/ha/Event.h PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/Event.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/FailoverExchange.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/ha/HaBroker.h 1506613 
  /trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/ha/Primary.h 1506613 
  /trunk/qpid/cpp/src/qpid/ha/Primary.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/ha/PrimaryTxObserver.h PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/PrimaryTxObserver.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.h 1506613 
  /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/ha/QueueSnapshots.h 1506613 
  /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.h 1506613 
  /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.h 1506613 
  /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/ha/TxReplicator.h PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/TxReplicator.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/qpid/ha/makeMessage.h 1506613 
  /trunk/qpid/cpp/src/qpid/ha/makeMessage.cpp 1506613 
  /trunk/qpid/cpp/src/qpid/ha/types.h 1506613 
  /trunk/qpid/cpp/src/qpid/ha/types.cpp 1506613 
  /trunk/qpid/cpp/src/tests/BrokerFixture.h 1506613 
  /trunk/qpid/cpp/src/tests/CMakeLists.txt 1506613 
  /trunk/qpid/cpp/src/tests/ClusterFixture.h PRE-CREATION 
  /trunk/qpid/cpp/src/tests/ClusterFixture.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/tests/ForkedBroker.h PRE-CREATION 
  /trunk/qpid/cpp/src/tests/ForkedBroker.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/tests/Makefile.am 1506613 
  /trunk/qpid/cpp/src/tests/MessagingFixture.h 1506613 
  /trunk/qpid/cpp/src/tests/TransactionObserverTest.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/tests/TxMocks.h 1506613 
  /trunk/qpid/cpp/src/tests/brokertest.py 1506613 
  /trunk/qpid/cpp/src/tests/ha_cpp_tests.cpp PRE-CREATION 
  /trunk/qpid/cpp/src/tests/ha_test.py 1506613 
  /trunk/qpid/cpp/src/tests/ha_tests.py 1506613 
  /trunk/qpid/cpp/src/tests/test_env.sh.in 1506613 
  /trunk/qpid/cpp/src/tests/test_tools.h 1506613 

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


Testing (updated)
-------

Basic tx replication appears to be working, used temporary log statements to prove the messages were being processed by the TxBuffer on the backup and not just being sent by the normal means.


Thanks,

Alan Conway


Re: Review Request 12289: QPID-4327: TransactionObserver interface

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
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: HA added PrimaryTransactionObserver.

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

> On July 11, 2013, 10:04 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h, line 59
> > <https://reviews.apache.org/r/12289/diff/4/?file=320029#file320029line59>
> >
> >     Is there a reason not to have QueuePtr and Message as the two arguments for dequeue as well as enqueue (and I do prefer those names as I think they are clearer in the transactional context).
> >     
> >     The Message would allow access to either sequence or replication id (or any other aspect that may be necessary).

Yes, I'm driving this from the TxBuffer and TxOps. The TxAccept op doesn't have message pointers, just DeliveryRecords. We can get the queue position and replication ID from those. We could use that to look up the message in the queue but in the case of HA that would be wasted effort since HA only needs to know the replication ID.

The names follow the same pattern as the TxOps, but I've no objection to changing them.


- Alan


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


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


Re: Review Request 12289: QPID-4327: HA TX transactions: basic replication.

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

> On July 11, 2013, 10:04 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h, line 59
> > <https://reviews.apache.org/r/12289/diff/4/?file=320029#file320029line59>
> >
> >     Is there a reason not to have QueuePtr and Message as the two arguments for dequeue as well as enqueue (and I do prefer those names as I think they are clearer in the transactional context).
> >     
> >     The Message would allow access to either sequence or replication id (or any other aspect that may be necessary).
> 
> Alan Conway wrote:
>     Yes, I'm driving this from the TxBuffer and TxOps. The TxAccept op doesn't have message pointers, just DeliveryRecords. We can get the queue position and replication ID from those. We could use that to look up the message in the queue but in the case of HA that would be wasted effort since HA only needs to know the replication ID.
>     
>     The names follow the same pattern as the TxOps, but I've no objection to changing them.

Looking good from fairly cursory inspection. I'm much happier with the observer interfaces now.


- Gordon


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


On July 31, 2013, 1:34 p.m., Alan Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12289/
> -----------------------------------------------------------
> 
> (Updated July 31, 2013, 1:34 p.m.)
> 
> 
> Review request for qpid, Andrew Stitcher and Gordon Sim.
> 
> 
> Bugs: QPID-4327
>     https://issues.apache.org/jira/browse/QPID-4327
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> QPID-4327: HA TX transactions: basic replication.
> 
> On primary a PrimaryTxObserver observes a transaction's TxBuffer and generates
> transaction events on a tx-replication-queue. On the backup a TxReplicator
> receives the events and constructs a TxBuffer equivalent to the one in the
> primary.
> 
> Unfinished:
> - Primary does not wait for backups to prepare() before committing.
> - All connected backups are assumed to be in the transaction, there are race
>   conditions around brokers joining/leavinv where this assumption is invalid.
> - Need more tests.
> 
> QPID-4327: HA get rid of Primary::get() singleton.
> 
> 
> QPID-4327: Added 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.
> 
> QPID-4327: Optimize brokertest.ready() to improve test runtimes.
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/include/qpid/framing/BufferTypes.h PRE-CREATION 
>   /trunk/qpid/cpp/src/CMakeLists.txt 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.h 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1508523 
>   /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 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/ConfigurationObservers.h 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/DtxAck.h 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/MessageStoreModule.h 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/MessageStoreModule.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/Queue.h 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/QueueRegistry.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/RecoveredDequeue.h 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/RecoveredEnqueue.h 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/broker/TxAccept.h 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/TxAccept.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/TxBuffer.h 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/TxBuffer.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/broker/TxOp.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/Backup.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/Event.h PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/ha/Event.cpp PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/ha/FailoverExchange.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/HaBroker.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/Membership.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/Membership.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/Primary.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/Primary.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/PrimaryTxObserver.h PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/ha/PrimaryTxObserver.cpp PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/QueueSnapshots.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/StatusCheck.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/TxReplicator.h PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/ha/TxReplicator.cpp PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/ha/makeMessage.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/makeMessage.cpp 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/types.h 1508523 
>   /trunk/qpid/cpp/src/qpid/ha/types.cpp 1508523 
>   /trunk/qpid/cpp/src/tests/BrokerFixture.h 1508523 
>   /trunk/qpid/cpp/src/tests/CMakeLists.txt 1508523 
>   /trunk/qpid/cpp/src/tests/MessagingFixture.h 1508523 
>   /trunk/qpid/cpp/src/tests/TransactionObserverTest.cpp PRE-CREATION 
>   /trunk/qpid/cpp/src/tests/TxMocks.h 1508523 
>   /trunk/qpid/cpp/src/tests/brokertest.py 1508523 
>   /trunk/qpid/cpp/src/tests/cluster_test.cpp PRE-CREATION 
>   /trunk/qpid/cpp/src/tests/ha_test.py 1508523 
>   /trunk/qpid/cpp/src/tests/ha_tests.py 1508523 
>   /trunk/qpid/cpp/src/tests/test_env.sh.in 1508523 
>   /trunk/qpid/cpp/src/tests/test_store.cpp 1508523 
>   /trunk/qpid/cpp/src/tests/test_tools.h 1508523 
> 
> Diff: https://reviews.apache.org/r/12289/diff/
> 
> 
> Testing
> -------
> 
> Basic tx replication appears to be working, used temporary log statements to prove the messages were being processed by the TxBuffer on the backup and not just being sent by the normal means.
> 
> 
> Thanks,
> 
> Alan Conway
> 
>


Re: Review Request 12289: QPID-4327: HA added PrimaryTransactionObserver.

Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12289/#review23009
-----------------------------------------------------------



/trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h
<https://reviews.apache.org/r/12289/#comment46779>

    Is there a reason not to have QueuePtr and Message as the two arguments for dequeue as well as enqueue (and I do prefer those names as I think they are clearer in the transactional context).
    
    The Message would allow access to either sequence or replication id (or any other aspect that may be necessary).


- Gordon Sim


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


Re: Review Request 12289: QPID-4327: HA added PrimaryTransactionObserver.

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
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.


Changes
-------

Modified TransactionObserver interface to use only queue and replication sequence numbers, not DeliveryRecords.


Repository: qpid


Description (updated)
-------

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 (updated)
-----

  /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: HA added PrimaryTransactionObserver.

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12289/
-----------------------------------------------------------

(Updated July 9, 2013, 9:29 p.m.)


Review request for qpid, Andrew Stitcher and Gordon Sim.


Changes
-------

I've 
- dropped TransactionObserverFactory
- renamed ConfigurationObserer as BrokerObserver
- added startTx points on BrokerObserver
- use those points to add a TransactionObserver to an individual transaction.

I think this is more consistent with our existing Observer practices


Summary (updated)
-----------------

QPID-4327: HA added PrimaryTransactionObserver.


Repository: qpid


Description (updated)
-------

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.


Diffs (updated)
-----

  /trunk/qpid/cpp/src/CMakeLists.txt 1500613 
  /trunk/qpid/cpp/src/Makefile.am 1500613 
  /trunk/qpid/cpp/src/ha.mk 1500613 
  /trunk/qpid/cpp/src/qpid/broker/Broker.h 1500613 
  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1500613 
  /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 1500613 
  /trunk/qpid/cpp/src/qpid/broker/ConfigurationObservers.h 1500613 
  /trunk/qpid/cpp/src/qpid/broker/DtxAck.h 1500613 
  /trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp 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/QueueRegistry.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/qpid/ha/Primary.h 1500613 
  /trunk/qpid/cpp/src/qpid/ha/Primary.cpp 1500613 
  /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 1500613 
  /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.h 1500613 
  /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.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.

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
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.

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

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

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: HA added PrimaryTransactionObserver.

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

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


Re: Review Request 12289: QPID-4327: TransactionObserver interface.

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

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

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


Re: Review Request 12289: QPID-4327: TransactionObserver interface.

Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
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_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.


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