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

2013-07-12 Thread Alan Conway 

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

2013-07-08 Thread Gordon Sim 


> On July 8, 2013, 12:10 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h, line 48
> > 
> >
> > 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) = 0;
> >   void dequeued(TransactionContext&, const Message&, const 
> > boost::shared_ptr) = 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://re

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

2013-07-08 Thread Alan Conway 

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

2013-07-08 Thread Alan Conway 


> On July 8, 2013, 12:10 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/TransactionObserver.h, line 48
> > 
> >
> > 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) = 0;
> >   void dequeued(TransactionContext&, const Message&, const 
> > boost::shared_ptr) = 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.

2013-07-08 Thread Gordon Sim 

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


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) = 0;
  void dequeued(TransactionContext&, const Message&, const 
boost::shared_ptr) = 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
> 
>