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