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 2011/05/26 22:53:00 UTC
Re: Review Request: 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.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-05-26 20:53:00.027175)
Review request for qpid, Andrew Stitcher, Alan Conway, and Gordon Sim.
Summary (updated)
-------
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 <ag...@redhat.com>.
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
Re: Review Request: 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.
Posted by Andy Goldstein <ag...@redhat.com>.
> 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?
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?
- Andy
-----------------------------------------------------------
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 <ag...@redhat.com>.
>
>
> 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
>
>
Re: Review Request: 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.
Posted by Andrew Stitcher <as...@apache.org>.
> 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 <ag...@redhat.com>.
>
>
> 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
>
>
Re: Review Request: 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.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review731
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp
<https://reviews.apache.org/r/791/#comment1476>
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?
- Andrew
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 <ag...@redhat.com>.
>
>
> 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
>
>
Re: Review Request: 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.
Posted by Andy Goldstein <ag...@redhat.com>.
> On 2011-05-26 21:16:54, Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/sys/Timer.h, line 52
> > <https://reviews.apache.org/r/791/diff/1/?file=19675#file19675line52>
> >
> > From mail trail: "We modified TimerTask slightly so that it would schedule the next firing when a cluster task's nextFireTime was in the future (in the even that the elder's deliverWakeup message was received prior to the local fire time)"
> >
> > Can you expand a little on why this was needed?
We found that setupNextFire() didn't work in non-elders for the following scenario:
- Use queue-purge-interval of 5 seconds for all nodes
- Start node 1 (the elder) at time T=0
- Start node 2 at T=2.5
- At T=5, node 1's cluster timer will fire and deliver the wakeup to all nodes
- Node 1 will execute the QueueCleaner task and successfully schedule the next fire to approx. T=10
- Node 2 will execute the QueueCleaner task, but it wasn't supposed to have fired yet (its time to fire was T=7.5)
- When node 2 tries to schedule its next fire time, it will fail and spit out the error message QPID_LOG(error, name << " couldn't setup next timer firing: " << Duration(nextFireTime, AbsTime::now()) << "[" << period << "]");
If/when node 2 becomes the elder, because setupNextFire() didn't succeed, the QueueCleaner task would remain cancelled and not run any more.
- Andy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review720
-----------------------------------------------------------
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 <ag...@redhat.com>.
>
>
> 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
>
>
Re: Review Request: 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.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review720
-----------------------------------------------------------
Andrew, can you review out the TimerTask fixes?
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp
<https://reviews.apache.org/r/791/#comment1460>
Moving the queue cleaner to the cluster timer was done before and reverted:
Author: Gordon Sim <gs...@apache.org>
Date: Wed Aug 11 06:06:24 2010
Revert commits r981517 and r981435 that moved periodic purging of
queues onto cluster's timer. If the timer fires during an update
it causes errors; it also puts a potentially time consuming task
on the clusters dispatch thread. Instead don't purge LVQs to avoid
cluster inconsistencies (and more directly the assertion that aims
to prevent these).
git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@984357 13f79535-47bb-0310-9956-ffa450edef68
However it seems that the cleaner does need to run under the cluster timer as the queue cleaner could cause policy limit inconsistencies.
Need to review the reason for reverting this and figure out the proper fix. This could be done in a separate commit or as part of this one.
/trunk/qpid/cpp/src/qpid/sys/Timer.h
<https://reviews.apache.org/r/791/#comment1461>
From mail trail: "We modified TimerTask slightly so that it would schedule the next firing when a cluster task's nextFireTime was in the future (in the even that the elder's deliverWakeup message was received prior to the local fire time)"
Can you expand a little on why this was needed?
- Alan
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 <ag...@redhat.com>.
>
>
> 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
>
>
Re: Review Request: 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.
Posted by Andy Goldstein <ag...@redhat.com>.
> On 2011-05-26 21:42:18, Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/sys/Timer.cpp, line 39
> > <https://reviews.apache.org/r/791/diff/1/?file=19676#file19676line39>
> >
> > I don't understand the purpose of fired:
> >
> > It looks like fired could only be set true by fireTask() and that will only happen if readyToFire() would return true.
> >
> > I can't see a way for fired to be set true until nextFireTime <= now().
> >
> > So it looks like in the test in line 65
> > (readToFire() || fired) || fired is superfluous.
See my comment above, but basically it's to cover the condition where the cluster timer in the elder forces a non elder task to fire "prematurely" (premature in the sense that the non-elder node thinks it should fire later). By setting fired to true in fireTask(), we guarantee that setupNextFire() will succeed in this case.
- Andy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review723
-----------------------------------------------------------
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 <ag...@redhat.com>.
>
>
> 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
>
>
Re: Review Request: 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.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review723
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp
<https://reviews.apache.org/r/791/#comment1465>
I don't understand the purpose of fired:
It looks like fired could only be set true by fireTask() and that will only happen if readyToFire() would return true.
I can't see a way for fired to be set true until nextFireTime <= now().
So it looks like in the test in line 65
(readToFire() || fired) || fired is superfluous.
- Andrew
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 <ag...@redhat.com>.
>
>
> 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
>
>
Re: Review Request: 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.
Posted by Alan Conway <ac...@redhat.com>.
> On 2011-05-30 18:36:30, Andy Goldstein wrote:
> > /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h, line 45
> > <https://reviews.apache.org/r/791/diff/2/?file=19793#file19793line45>
> >
> > I don't think this method is needed any more
Yep
> On 2011-05-30 18:36:30, Andy Goldstein wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Message.cpp, line 379
> > <https://reviews.apache.org/r/791/diff/2/?file=19795#file19795line379>
> >
> > The expiration time should be calculated using the cluster clock (via e.getCurrentTime()), not the broker's local system clock...right? Otherwise, each node may think the message expires at a different time, leading to inconsistencies. It looks like this is done in adjustTtl, but not here.
Yikes! I'll review the changes carefully to make sure I didn't slip up anywhere else and post a new patch.
> On 2011-05-30 18:36:30, Andy Goldstein wrote:
> > /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h, line 47
> > <https://reviews.apache.org/r/791/diff/2/?file=19793#file19793line47>
> >
> > I don't think this method is needed any more
Yep
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review735
-----------------------------------------------------------
On 2011-05-30 17:44:11, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/791/
> -----------------------------------------------------------
>
> (Updated 2011-05-30 17:44:11)
>
>
> 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.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
> /trunk/qpid/cpp/xml/cluster.xml 1128070
> /trunk/qpid/python/examples/api/spout 1128070
>
> Diff: https://reviews.apache.org/r/791/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: 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.
Posted by Andy Goldstein <ag...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review735
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h
<https://reviews.apache.org/r/791/#comment1481>
I don't think this method is needed any more
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h
<https://reviews.apache.org/r/791/#comment1482>
I don't think this method is needed any more
/trunk/qpid/cpp/src/qpid/broker/Message.cpp
<https://reviews.apache.org/r/791/#comment1480>
The expiration time should be calculated using the cluster clock (via e.getCurrentTime()), not the broker's local system clock...right? Otherwise, each node may think the message expires at a different time, leading to inconsistencies. It looks like this is done in adjustTtl, but not here.
- Andy
On 2011-05-30 17:44:11, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/791/
> -----------------------------------------------------------
>
> (Updated 2011-05-30 17:44:11)
>
>
> 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.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
> /trunk/qpid/cpp/xml/cluster.xml 1128070
> /trunk/qpid/python/examples/api/spout 1128070
>
> Diff: https://reviews.apache.org/r/791/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: 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.
Posted by Alan Conway <ac...@redhat.com>.
> On 2011-05-31 15:35:19, Andy Goldstein wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Message.cpp, line 374
> > <https://reviews.apache.org/r/791/diff/3/?file=20023#file20023line374>
> >
> > I had put a TODO/question in my original patch asking if this time needed to be based on the cluster time, but I didn't change the code here (talking only about props->setExpiration) because it didn't look like the broker/cluster used the expiration value from the message properties...should this be based on the cluster time too, or does it not matter?
It doesn't really matter in that the expiration property is only used by the client receiving the message. It may be more accurate to use the real time. I'll add a comment to the code explaining why we don't use cluster time here.
> On 2011-05-31 15:35:19, Andy Goldstein wrote:
> > /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp, line 1187
> > <https://reviews.apache.org/r/791/diff/3/?file=20027#file20027line1187>
> >
> > maybe add a comment that this method is called as part of the initial state transfer when joining a cluster
done
> On 2011-05-31 15:35:19, Andy Goldstein wrote:
> > /trunk/qpid/python/examples/api/spout, line 50
> > <https://reviews.apache.org/r/791/diff/3/?file=20040#file20040line50>
> >
> > I added this to help me out w/testing - there is already an open JIRA (QPID-2890) to add support for TTL (among other things) in spout, and the patch proposes -L for TTL, so this would conflict. Probably want to coordinate or just drop this from my patch and pull in the patch for 2890 separately
Dropped. BTW there are a pair of c++ clients: qpid-send and qpid-receive that have TTL support.
I am adding a test for this, once that's done I'll post a new patch.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review737
-----------------------------------------------------------
On 2011-05-31 21:37:57, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/791/
> -----------------------------------------------------------
>
> (Updated 2011-05-31 21:37:57)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
>
>
> 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.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
> /trunk/qpid/cpp/src/tests/cluster_tests.py 1128070
> /trunk/qpid/cpp/xml/cluster.xml 1128070
>
> Diff: https://reviews.apache.org/r/791/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: 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.
Posted by Andy Goldstein <ag...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review737
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/Message.cpp
<https://reviews.apache.org/r/791/#comment1486>
I had put a TODO/question in my original patch asking if this time needed to be based on the cluster time, but I didn't change the code here (talking only about props->setExpiration) because it didn't look like the broker/cluster used the expiration value from the message properties...should this be based on the cluster time too, or does it not matter?
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp
<https://reviews.apache.org/r/791/#comment1487>
maybe add a comment that this method is called as part of the initial state transfer when joining a cluster
/trunk/qpid/python/examples/api/spout
<https://reviews.apache.org/r/791/#comment1488>
I added this to help me out w/testing - there is already an open JIRA (QPID-2890) to add support for TTL (among other things) in spout, and the patch proposes -L for TTL, so this would conflict. Probably want to coordinate or just drop this from my patch and pull in the patch for 2890 separately
- Andy
On 2011-05-31 15:09:06, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/791/
> -----------------------------------------------------------
>
> (Updated 2011-05-31 15:09:06)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
>
>
> 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.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
> /trunk/qpid/cpp/xml/cluster.xml 1128070
> /trunk/qpid/python/examples/api/spout 1128070
>
> Diff: https://reviews.apache.org/r/791/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: QPID-3280: Performance problem with TTL messages.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-06-10 21:18:44.812882)
Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
Changes
-------
Hopefully the last patch in this series.
Summary (updated)
-------
QPID-3280: Performance problem with TTL messages.
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.
The previous approach to TTL in the cluster is replaced with a simpler
"cluster clock". Also QueueCleaner is executed in the cluster timer,
and modified to be deterministic in a cluster.
Diffs (updated)
-----
/trunk/qpid/cpp/include/qpid/framing/FieldTable.h 1134238
/trunk/qpid/cpp/src/CMakeLists.txt 1134238
/trunk/qpid/cpp/src/Makefile.am 1134238
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1134238
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1134238
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1134238
/trunk/qpid/cpp/src/qpid/broker/Message.h 1134238
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1134238
/trunk/qpid/cpp/src/qpid/broker/Queue.h 1134238
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1134238
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1134238
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1134238
/trunk/qpid/cpp/src/qpid/broker/RateTracker.h 1134238
/trunk/qpid/cpp/src/qpid/broker/RateTracker.cpp 1134238
/trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1134238
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1134238
/trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1134238
/trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1134238
/trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1134238
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1134238
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1134238
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1134238
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1134238
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1134238
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1134238
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1134238
/trunk/qpid/cpp/src/qpid/framing/AMQHeaderBody.h 1134238
/trunk/qpid/cpp/src/qpid/sys/Timer.h 1134238
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1134238
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1134238
/trunk/qpid/cpp/src/tests/cluster_test_logs.py 1134238
/trunk/qpid/cpp/src/tests/cluster_tests.py 1134238
/trunk/qpid/cpp/xml/cluster.xml 1134238
Diff: https://reviews.apache.org/r/791/diff
Testing
-------
Thanks,
Alan
Re: Review Request: WIP: Misc fixes to cluster TTL
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-06-08 17:47:17.142625)
Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
Changes
-------
I've made a couple more fixes and rebased to latest trunk. There are 2 issues with the patch:
- in repeated runs (5-6) I've seen test_ttl_failover and test_failover fail with a broker exiting with nothing abnormal in the logs and no core file.
- performance is about 15% better for messages with TTL, but 10% worse for messages without TTL.
I'm a bit stuck on the test failures, any help appreciated. I think we probably have to optimize the non-TTL case a bit more.
Summary (updated)
-------
WIP: Misc fixes to cluster TTL
- Fix issue with empty message-properties being added to messages.
- Remove queue cleaner use of system clock
- Fix test bugs
WIP: Use atomic counter and period in place of RateTracker for QueueCleaner.
RateTracker was using the non-cluster clock to decide to run purges or not.
This is a clock-free solution that gives the same result and is safe in cluster.
WIP: agoldste's expiration replication fix.
QPID-3280: Fixed extra failover update messages on brokers joining the cluster.
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.
Diffs (updated)
-----
/trunk/qpid/cpp/include/qpid/framing/FieldTable.h 1133166
/trunk/qpid/cpp/src/CMakeLists.txt 1133166
/trunk/qpid/cpp/src/Makefile.am 1133166
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1133166
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1133166
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1133166
/trunk/qpid/cpp/src/qpid/broker/Message.h 1133166
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1133166
/trunk/qpid/cpp/src/qpid/broker/Queue.h 1133166
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1133166
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1133166
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1133166
/trunk/qpid/cpp/src/qpid/broker/RateTracker.h 1133166
/trunk/qpid/cpp/src/qpid/broker/RateTracker.cpp 1133166
/trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1133166
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1133166
/trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1133166
/trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1133166
/trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1133166
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1133166
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1133166
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1133166
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1133166
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1133166
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1133166
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1133166
/trunk/qpid/cpp/src/qpid/framing/AMQHeaderBody.h 1133166
/trunk/qpid/cpp/src/qpid/sys/Timer.h 1133166
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1133166
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1133166
/trunk/qpid/cpp/src/tests/cluster_test_logs.py 1133166
/trunk/qpid/cpp/src/tests/cluster_tests.py 1133166
/trunk/qpid/cpp/xml/cluster.xml 1133166
Diff: https://reviews.apache.org/r/791/diff
Testing
-------
Thanks,
Alan
Re: Review Request: WIP: Fix issue with empty message-properties being added
to messages.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-06-03 21:43:43.083444)
Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
Changes
-------
I think this might be the Last Patch.
Summary (updated)
-------
WIP: Fix issue with empty message-properties being added to messages.
WIP: Use atomic counter and period in place of RateTracker for QueueCleaner.
RateTracker was using the non-cluster clock to decide to run purges or not.
This is a clock-free solution that gives the same result and is safe in cluster.
WIP: agoldste's expiration replication fix.
QPID-3280: Fixed extra failover update messages on brokers joining the cluster.
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.
Diffs (updated)
-----
/trunk/qpid/cpp/include/qpid/framing/FieldTable.h 1131084
/trunk/qpid/cpp/managementgen/Makefile.am 1128070
/trunk/qpid/cpp/src/CMakeLists.txt 1131084
/trunk/qpid/cpp/src/Makefile.am 1131084
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1131084
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/Message.h 1131084
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/Queue.h 1131084
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1131084
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/RateTracker.h 1131084
/trunk/qpid/cpp/src/qpid/broker/RateTracker.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1131084
/trunk/qpid/cpp/src/qpid/framing/AMQHeaderBody.h 1131084
/trunk/qpid/cpp/src/qpid/sys/Timer.h 1131084
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1131084
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1131084
/trunk/qpid/cpp/src/tests/cluster_test_logs.py 1131084
/trunk/qpid/cpp/src/tests/cluster_tests.py 1131084
/trunk/qpid/cpp/xml/cluster.xml 1131084
Diff: https://reviews.apache.org/r/791/diff
Testing
-------
Thanks,
Alan
Re: Review Request: QPID-3280: Efficient handling of TTL by cluster
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-06-03 20:30:03.292570)
Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
Summary (updated)
-------
QPID-3280: Efficient handling of TTL by cluster
WIP: Use atomic counter and period in place of RateTracker for QueueCleaner.
RateTracker was using the non-cluster clock to decide to run purges or not.
This is a clock-free solution that gives the same result and is safe in cluster.
WIP: agoldste's expiration replication fix.
QPID-3280: Fixed extra failover update messages on brokers joining the cluster.
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.
Diffs
-----
/trunk/qpid/cpp/managementgen/Makefile.am 1128070
/trunk/qpid/cpp/src/CMakeLists.txt 1131084
/trunk/qpid/cpp/src/Makefile.am 1131084
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1131084
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/Message.h 1131084
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/Queue.h 1131084
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1131084
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/RateTracker.h 1131084
/trunk/qpid/cpp/src/qpid/broker/RateTracker.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1131084
/trunk/qpid/cpp/src/qpid/sys/Timer.h 1131084
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1131084
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1131084
/trunk/qpid/cpp/src/tests/cluster_test_logs.py 1131084
/trunk/qpid/cpp/src/tests/cluster_tests.py 1131084
/trunk/qpid/cpp/xml/cluster.xml 1131084
Diff: https://reviews.apache.org/r/791/diff
Testing
-------
Thanks,
Alan
Re: Review Request: WIP: Use atomic counter and period in place of
RateTracker for QueueCleaner.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-06-03 20:29:32.765294)
Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
Changes
-------
This patch has the improvements below. It is passing the ttl test but failing 2 other tests in make check. I suspect the extra header is modifying the message size somewhere. The failures are:
- cluster_tests.ShortTests.test_store_direct_update_match
- It is passing the ttl test but failing 2 other tests in make check.
Summary (updated)
-------
QPID-3280: Efficient handling of TTL by cluster
WIP: Use atomic counter and period in place of RateTracker for QueueCleaner.
RateTracker was using the non-cluster clock to decide to run purges or not.
This is a clock-free solution that gives the same result and is safe in cluster.
WIP: agoldste's expiration replication fix.
QPID-3280: Fixed extra failover update messages on brokers joining the cluster.
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.
Diffs (updated)
-----
/trunk/qpid/cpp/managementgen/Makefile.am 1128070
/trunk/qpid/cpp/src/CMakeLists.txt 1131084
/trunk/qpid/cpp/src/Makefile.am 1131084
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1131084
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/Message.h 1131084
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/Queue.h 1131084
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1131084
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1131084
/trunk/qpid/cpp/src/qpid/broker/RateTracker.h 1131084
/trunk/qpid/cpp/src/qpid/broker/RateTracker.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1131084
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1131084
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1131084
/trunk/qpid/cpp/src/qpid/sys/Timer.h 1131084
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1131084
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1131084
/trunk/qpid/cpp/src/tests/cluster_test_logs.py 1131084
/trunk/qpid/cpp/src/tests/cluster_tests.py 1131084
/trunk/qpid/cpp/xml/cluster.xml 1131084
Diff: https://reviews.apache.org/r/791/diff
Testing
-------
Thanks,
Alan
Re: Review Request: QPID-3280: Fixed extra failover update messages on
brokers joining the cluster.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-06-02 20:28:13.387392)
Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
Changes
-------
Fixed a bug that caused inconsistencies in the number of messages on failover-update queues.
Still intermittently failing the ttl_failover_test, with an inconsistency in the number of messages on queue "q". It appears that in some cases messages are being expired by the queue cleaner on one broker but by consume on another. I'm chasing this down, if anyone has any insights let me know.
Summary (updated)
-------
QPID-3280: Fixed extra failover update messages on brokers joining the cluster.
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.
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
/trunk/qpid/cpp/src/tests/cluster_test_logs.py 1128070
/trunk/qpid/cpp/src/tests/cluster_tests.py 1128070
/trunk/qpid/cpp/xml/cluster.xml 1128070
Diff: https://reviews.apache.org/r/791/diff
Testing
-------
Thanks,
Alan
Re: Review Request: 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.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-06-01 17:58:05.198586)
Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
Changes
-------
Changes:
- Gordons suggestion for Message::adjustTtl
- Removed python logging noise from tests.
- Fixed some test race conditions.
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.
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
/trunk/qpid/cpp/src/tests/cluster_tests.py 1128070
/trunk/qpid/cpp/xml/cluster.xml 1128070
Diff: https://reviews.apache.org/r/791/diff
Testing
-------
Thanks,
Alan
Re: Review Request: 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.
Posted by Alan Conway <ac...@redhat.com>.
> On 2011-06-01 09:28:27, Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Message.cpp, line 391
> > <https://reviews.apache.org/r/791/diff/4/?file=20056#file20056line391>
> >
> > Not saying this is wrong, but perhaps a fraction safer if the TTL is always adjusted and the expiry policy is tested only in order to determine if it should be used to get the current time. I.e.
> >
> > if (props->getTtl()) {
> > sys::Mutex::ScopedLock l(lock);
> > if (expiration < FAR_FUTURE) {
> > sys::Duration current =
> > expiryPolicy ?
> > expiryPolicy->getCurrentTime()
> > : sys::AbsTime::now();
> > sys::Duration d(current, getExpiration());
> > props->setTtl(...)
> > }
> > }
Done, it pays to be safe :)
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review743
-----------------------------------------------------------
On 2011-05-31 21:37:57, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/791/
> -----------------------------------------------------------
>
> (Updated 2011-05-31 21:37:57)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
>
>
> 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.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
> /trunk/qpid/cpp/src/tests/cluster_tests.py 1128070
> /trunk/qpid/cpp/xml/cluster.xml 1128070
>
> Diff: https://reviews.apache.org/r/791/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: 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.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review743
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/Message.cpp
<https://reviews.apache.org/r/791/#comment1518>
Not saying this is wrong, but perhaps a fraction safer if the TTL is always adjusted and the expiry policy is tested only in order to determine if it should be used to get the current time. I.e.
if (props->getTtl()) {
sys::Mutex::ScopedLock l(lock);
if (expiration < FAR_FUTURE) {
sys::Duration current =
expiryPolicy ?
expiryPolicy->getCurrentTime()
: sys::AbsTime::now();
sys::Duration d(current, getExpiration());
props->setTtl(...)
}
}
- Gordon
On 2011-05-31 21:37:57, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/791/
> -----------------------------------------------------------
>
> (Updated 2011-05-31 21:37:57)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
>
>
> 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.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
> /trunk/qpid/cpp/src/tests/cluster_tests.py 1128070
> /trunk/qpid/cpp/xml/cluster.xml 1128070
>
> Diff: https://reviews.apache.org/r/791/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: 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.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/#review744
-----------------------------------------------------------
Ship it!
Looks fine to me.
- Gordon
On 2011-05-31 21:37:57, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/791/
> -----------------------------------------------------------
>
> (Updated 2011-05-31 21:37:57)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
>
>
> 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.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
> /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
> /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
> /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
> /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
> /trunk/qpid/cpp/src/tests/cluster_tests.py 1128070
> /trunk/qpid/cpp/xml/cluster.xml 1128070
>
> Diff: https://reviews.apache.org/r/791/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: 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.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-05-31 21:37:57.813235)
Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
Changes
-------
Added a stress test, its causing an 'invalid argument' error. Will hunt it down.
To run the test quickly, in the tests directory: source test_env.sh; run_cluster_tests *ttl* -DDURATION=4 -vERROR
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.
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
/trunk/qpid/cpp/src/tests/cluster_tests.py 1128070
/trunk/qpid/cpp/xml/cluster.xml 1128070
Diff: https://reviews.apache.org/r/791/diff
Testing
-------
Thanks,
Alan
Re: Review Request: 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.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-05-31 15:09:06.555345)
Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.
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.
Diffs
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
/trunk/qpid/cpp/xml/cluster.xml 1128070
/trunk/qpid/python/examples/api/spout 1128070
Diff: https://reviews.apache.org/r/791/diff
Testing
-------
Thanks,
Alan
Re: Review Request: 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.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-05-31 15:08:36.979484)
Review request for qpid, Andrew Stitcher, Alan Conway, and Gordon Sim.
Changes
-------
Fixed issues pointed out by Andy.
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.
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
/trunk/qpid/cpp/xml/cluster.xml 1128070
/trunk/qpid/python/examples/api/spout 1128070
Diff: https://reviews.apache.org/r/791/diff
Testing
-------
Thanks,
Alan
Re: Review Request: 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.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------
(Updated 2011-05-30 17:44:11.319791)
Review request for qpid, Andrew Stitcher, Alan Conway, and Gordon Sim.
Summary (updated)
-------
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.
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070
/trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070
/trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070
/trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070
/trunk/qpid/cpp/xml/cluster.xml 1128070
/trunk/qpid/python/examples/api/spout 1128070
Diff: https://reviews.apache.org/r/791/diff
Testing
-------
Thanks,
Alan