> On 2011-05-27 16:59:51, Andrew Stitcher wrote: > > /trunk/qpid/cpp/src/qpid/sys/Timer.cpp, line 65 > > <https://reviews.apache.org/r/791/diff/1/?file=19676#file19676line65> > > > > As my previous comment I think this test should now be: > > > > if (period && fired) { > > > > And as this seems to be the only user of readyToFire() it could be > > removed > > > > However I think this might be the wrong behaviour in any case. > > > > If a periodic event is preempted by the elder why isn't the elder going > > to preempt the next firing too. In other words why are we setting up a next > > firing on the local timer when the elders timer is going to do the work for > > us? > > Andy Goldstein wrote: > if (period && fired) seems cleaner to me too. > > Presumably the elder will preempt the next firing too, but the task (in > this case, QueueCleaner), isn't aware that it was run in a non-elder was it? > The general pattern for periodic tasks seems to be > > 1) add to timer > 2) timer fires task > 3) as part of task's fire() method, call setupNextFire() and then add the > task to the timer to reschedule it > > Is there some way to avoid step 3) if you're not the elder? Does this > make sense?
I see. I think it probably means that the TimerTask class needs to have a clustered counterpart as well as Timer so that when the a TimerTask is added to a ClusterTimer the ClusterTimer transparently creates a ClusterTimerTask wrapping a TimerTask and overrides TimerTasks member functions to do "the right thing". This seems like a design issue in the ClusterTimer rather than this fix specifically - Alan what do you think? - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/791/#review731 ----------------------------------------------------------- On 2011-05-26 20:53:00, Alan Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/791/ > ----------------------------------------------------------- > > (Updated 2011-05-26 20:53:00) > > > Review request for qpid, Andrew Stitcher, Alan Conway, and Gordon Sim. > > > Summary > ------- > > QPID-3280: When sending a large number of messages with nonzero TTLs to a > cluster, overall message throughput drops by around 20-30% compared to > messages with TTL 0. > > Replaced the complicated message expirly logic in the cluster with a simpler > "cluster clock" for expiry of messages with TTL. > > Patch supplied by Andy Goldstein <[email protected]>. > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128002 > /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128002 > /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128002 > /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128002 > /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128002 > /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128002 > /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128002 > /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128002 > /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128002 > /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128002 > /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128002 > /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128002 > /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128002 > /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128002 > /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128002 > /trunk/qpid/cpp/src/qpid/sys/Timer.h 1128002 > /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128002 > /trunk/qpid/cpp/xml/cluster.xml 1128002 > /trunk/qpid/python/examples/api/spout 1128002 > > Diff: https://reviews.apache.org/r/791/diff > > > Testing > ------- > > > Thanks, > > Alan > >
