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/12/13 16:16:31 UTC
Review Request 16245: NO-JIRA: Refactor: clean up QueeuObservers.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16245/
-----------------------------------------------------------
Review request for qpid and Gordon Sim.
Repository: qpid
Description
-------
NO-JIRA: Refactor: clean up QueeuObservers.
Refactor of queue observers to use the broker::Observers base class. Simplifies Queue code
and makes it more consistent with other observers (BrokerObservers, ConnectionObservers.)
Modified Observers base class to allow identical locking behaviour to previous
impementation.
Diffs
-----
/trunk/qpid/cpp/src/qpid/broker/BrokerObservers.h 1550190
/trunk/qpid/cpp/src/qpid/broker/ConnectionObservers.h 1550190
/trunk/qpid/cpp/src/qpid/broker/Observers.h 1550190
/trunk/qpid/cpp/src/qpid/broker/Queue.h 1550190
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1550190
/trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1550190
/trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1550190
/trunk/qpid/cpp/src/qpid/broker/QueueObservers.h PRE-CREATION
/trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1550190
/trunk/qpid/cpp/src/qpid/ha/QueueGuard.cpp 1550190
/trunk/qpid/cpp/src/qpid/ha/QueueReplicator.cpp 1550190
/trunk/qpid/cpp/src/qpid/ha/QueueSnapshots.h 1550190
/trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1550190
Diff: https://reviews.apache.org/r/16245/diff/
Testing
-------
Full ctest
Thanks,
Alan Conway
Re: Review Request 16245: NO-JIRA: Refactor: clean up QueeuObservers.
Posted by Alan Conway <ac...@redhat.com>.
> On Dec. 13, 2013, 4:53 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Observers.h, line 55
> > <https://reviews.apache.org/r/16245/diff/1/?file=397491#file397491line55>
> >
> > For queue 'events' this would take a copy of all observers on each such event, whereas before this the observers would have been iterated over under a lock, right?
> >
> > This means that we copy the set for every enqueue and dequeue for example. Is there any concern that would affect performance?
> >
> > Could we instead have two versions of each, one where the lock is passed in, avoiding the need to copy? If I'm not mistaken, it is held in any case across the each() call for enqueue/dequeue.
What a good idea :) If you look further down you'll see there is a protected version of each() which does iterate under the lock, this is the one used by QueueObservers, so the locking behaviour should be identical.
The copy version of each() is used by the other observers (BrokerObserver, ConnectionObserver) which was the case before this change.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16245/#review30344
-----------------------------------------------------------
On Dec. 13, 2013, 3:16 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16245/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2013, 3:16 p.m.)
>
>
> Review request for qpid and Gordon Sim.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> NO-JIRA: Refactor: clean up QueeuObservers.
>
> Refactor of queue observers to use the broker::Observers base class. Simplifies Queue code
> and makes it more consistent with other observers (BrokerObservers, ConnectionObservers.)
>
> Modified Observers base class to allow identical locking behaviour to previous
> impementation.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/BrokerObservers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/ConnectionObservers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Observers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Queue.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueObservers.h PRE-CREATION
> /trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueGuard.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueSnapshots.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1550190
>
> Diff: https://reviews.apache.org/r/16245/diff/
>
>
> Testing
> -------
>
> Full ctest
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 16245: QPID-5421: HA replication error in stand-alone
replication
Posted by Gordon Sim <gs...@redhat.com>.
> On Dec. 13, 2013, 4:53 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Observers.h, line 55
> > <https://reviews.apache.org/r/16245/diff/1/?file=397491#file397491line55>
> >
> > For queue 'events' this would take a copy of all observers on each such event, whereas before this the observers would have been iterated over under a lock, right?
> >
> > This means that we copy the set for every enqueue and dequeue for example. Is there any concern that would affect performance?
> >
> > Could we instead have two versions of each, one where the lock is passed in, avoiding the need to copy? If I'm not mistaken, it is held in any case across the each() call for enqueue/dequeue.
>
> Alan Conway wrote:
> What a good idea :) If you look further down you'll see there is a protected version of each() which does iterate under the lock, this is the one used by QueueObservers, so the locking behaviour should be identical.
>
> The copy version of each() is used by the other observers (BrokerObserver, ConnectionObserver) which was the case before this change.
Oops, sorry! I should have noticed that.
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16245/#review30344
-----------------------------------------------------------
On Dec. 13, 2013, 5:11 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16245/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2013, 5:11 p.m.)
>
>
> Review request for qpid and Gordon Sim.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> This is the reason why I did the observer refactor.
>
> A while back you pointed out that it would be simpler to eliminate IdSetter and have the Queue set the replication IDs on messages. However there's a problem with that: On the primary you want the queue setting the IDs, but on the backup you don't - you want the QueueReplicator setting the IDs based on what it's getting from the ReplicatingSubscription. But then when that backup is promoted, you want the queue to start setting the IDs again. Using an observer makes it easy to switch the behaviour on and off. Can you think of a neater way?
>
>
> QPID-5421: HA replication error in stand-alone replication
>
> There were replication errors because with stand-alone replication an IdSetter
> was not set on the original queue until queue replication was set up. Any
> messages on the queue *before* replication was setup had 0 replication IDs. When
> one of those messages was dequeued on the source queue, an incorrect message was
> dequeued on the replica queue.
>
> The fix is to add an IdSetter to every queue when replication is enabled.
>
> The unit test ha_tests.ReplicationTests.test_standalone_queue_replica has been
> updated to test for this issue.
>
> This commit also has some general tidy-up work around IdSetter and QueueSnapshot.
>
> QPID-5421: Refactor: clean up QueueObservers.
>
> Refactor of queue observers to use the broker::Observers base class. Simplifies Queue code
> and makes it more consistent with other observers (BrokerObservers, ConnectionObservers.)
>
> Modified Observers base class to allow identical locking behaviour to previous
> impementation.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/BrokerObservers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/ConnectionObservers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Observers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Queue.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueObservers.h PRE-CREATION
> /trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/HaBroker.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/IdSetter.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/Primary.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/Primary.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueGuard.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueSnapshot.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueSnapshots.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1550190
> /trunk/qpid/cpp/src/tests/ha_tests.py 1550190
>
> Diff: https://reviews.apache.org/r/16245/diff/
>
>
> Testing
> -------
>
> Full ctest
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 16245: NO-JIRA: Refactor: clean up QueeuObservers.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16245/#review30344
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/Observers.h
<https://reviews.apache.org/r/16245/#comment58072>
For queue 'events' this would take a copy of all observers on each such event, whereas before this the observers would have been iterated over under a lock, right?
This means that we copy the set for every enqueue and dequeue for example. Is there any concern that would affect performance?
Could we instead have two versions of each, one where the lock is passed in, avoiding the need to copy? If I'm not mistaken, it is held in any case across the each() call for enqueue/dequeue.
- Gordon Sim
On Dec. 13, 2013, 3:16 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16245/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2013, 3:16 p.m.)
>
>
> Review request for qpid and Gordon Sim.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> NO-JIRA: Refactor: clean up QueeuObservers.
>
> Refactor of queue observers to use the broker::Observers base class. Simplifies Queue code
> and makes it more consistent with other observers (BrokerObservers, ConnectionObservers.)
>
> Modified Observers base class to allow identical locking behaviour to previous
> impementation.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/BrokerObservers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/ConnectionObservers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Observers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Queue.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueObservers.h PRE-CREATION
> /trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueGuard.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueSnapshots.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1550190
>
> Diff: https://reviews.apache.org/r/16245/diff/
>
>
> Testing
> -------
>
> Full ctest
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 16245: QPID-5421: HA replication error in stand-alone
replication
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16245/#review30350
-----------------------------------------------------------
Ship it!
Ship It!
- Gordon Sim
On Dec. 13, 2013, 5:11 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16245/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2013, 5:11 p.m.)
>
>
> Review request for qpid and Gordon Sim.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> This is the reason why I did the observer refactor.
>
> A while back you pointed out that it would be simpler to eliminate IdSetter and have the Queue set the replication IDs on messages. However there's a problem with that: On the primary you want the queue setting the IDs, but on the backup you don't - you want the QueueReplicator setting the IDs based on what it's getting from the ReplicatingSubscription. But then when that backup is promoted, you want the queue to start setting the IDs again. Using an observer makes it easy to switch the behaviour on and off. Can you think of a neater way?
>
>
> QPID-5421: HA replication error in stand-alone replication
>
> There were replication errors because with stand-alone replication an IdSetter
> was not set on the original queue until queue replication was set up. Any
> messages on the queue *before* replication was setup had 0 replication IDs. When
> one of those messages was dequeued on the source queue, an incorrect message was
> dequeued on the replica queue.
>
> The fix is to add an IdSetter to every queue when replication is enabled.
>
> The unit test ha_tests.ReplicationTests.test_standalone_queue_replica has been
> updated to test for this issue.
>
> This commit also has some general tidy-up work around IdSetter and QueueSnapshot.
>
> QPID-5421: Refactor: clean up QueueObservers.
>
> Refactor of queue observers to use the broker::Observers base class. Simplifies Queue code
> and makes it more consistent with other observers (BrokerObservers, ConnectionObservers.)
>
> Modified Observers base class to allow identical locking behaviour to previous
> impementation.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/BrokerObservers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/ConnectionObservers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Observers.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Queue.h 1550190
> /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1550190
> /trunk/qpid/cpp/src/qpid/broker/QueueObservers.h PRE-CREATION
> /trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/HaBroker.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/IdSetter.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/Primary.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/Primary.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueGuard.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.cpp 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueSnapshot.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/QueueSnapshots.h 1550190
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1550190
> /trunk/qpid/cpp/src/tests/ha_tests.py 1550190
>
> Diff: https://reviews.apache.org/r/16245/diff/
>
>
> Testing
> -------
>
> Full ctest
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 16245: QPID-5421: HA replication error in stand-alone
replication
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16245/
-----------------------------------------------------------
(Updated Dec. 13, 2013, 5:11 p.m.)
Review request for qpid and Gordon Sim.
Summary (updated)
-----------------
QPID-5421: HA replication error in stand-alone replication
Repository: qpid
Description (updated)
-------
This is the reason why I did the observer refactor.
A while back you pointed out that it would be simpler to eliminate IdSetter and have the Queue set the replication IDs on messages. However there's a problem with that: On the primary you want the queue setting the IDs, but on the backup you don't - you want the QueueReplicator setting the IDs based on what it's getting from the ReplicatingSubscription. But then when that backup is promoted, you want the queue to start setting the IDs again. Using an observer makes it easy to switch the behaviour on and off. Can you think of a neater way?
QPID-5421: HA replication error in stand-alone replication
There were replication errors because with stand-alone replication an IdSetter
was not set on the original queue until queue replication was set up. Any
messages on the queue *before* replication was setup had 0 replication IDs. When
one of those messages was dequeued on the source queue, an incorrect message was
dequeued on the replica queue.
The fix is to add an IdSetter to every queue when replication is enabled.
The unit test ha_tests.ReplicationTests.test_standalone_queue_replica has been
updated to test for this issue.
This commit also has some general tidy-up work around IdSetter and QueueSnapshot.
QPID-5421: Refactor: clean up QueueObservers.
Refactor of queue observers to use the broker::Observers base class. Simplifies Queue code
and makes it more consistent with other observers (BrokerObservers, ConnectionObservers.)
Modified Observers base class to allow identical locking behaviour to previous
impementation.
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/BrokerObservers.h 1550190
/trunk/qpid/cpp/src/qpid/broker/ConnectionObservers.h 1550190
/trunk/qpid/cpp/src/qpid/broker/Observers.h 1550190
/trunk/qpid/cpp/src/qpid/broker/Queue.h 1550190
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1550190
/trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1550190
/trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1550190
/trunk/qpid/cpp/src/qpid/broker/QueueObservers.h PRE-CREATION
/trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1550190
/trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.cpp 1550190
/trunk/qpid/cpp/src/qpid/ha/HaBroker.h 1550190
/trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1550190
/trunk/qpid/cpp/src/qpid/ha/IdSetter.h 1550190
/trunk/qpid/cpp/src/qpid/ha/Primary.h 1550190
/trunk/qpid/cpp/src/qpid/ha/Primary.cpp 1550190
/trunk/qpid/cpp/src/qpid/ha/QueueGuard.cpp 1550190
/trunk/qpid/cpp/src/qpid/ha/QueueReplicator.h 1550190
/trunk/qpid/cpp/src/qpid/ha/QueueReplicator.cpp 1550190
/trunk/qpid/cpp/src/qpid/ha/QueueSnapshot.h 1550190
/trunk/qpid/cpp/src/qpid/ha/QueueSnapshots.h 1550190
/trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1550190
/trunk/qpid/cpp/src/tests/ha_tests.py 1550190
Diff: https://reviews.apache.org/r/16245/diff/
Testing
-------
Full ctest
Thanks,
Alan Conway