qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gordon Sim" <g...@redhat.com>
Subject Re: Review Request 21153: [C++ broker] Make Queue::purgeExpired more efficient by calling AbsTime::now() just once
Date Mon, 12 May 2014 08:56:28 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21153/#review42675
-----------------------------------------------------------


Looks good to me. Might be worth a comment in the new ExpiryPolicy method, explaining why
the time is passed in. I'm not sure what function the expiry policy serves anymore anyway.
It used to be used in the old cluster to eliminate a source of indeterminacy. Not sure if
it is needed anymore... (without it, could simply have the message return its expiration and
do comparison against any time suitable).

- Gordon Sim


On May 7, 2014, 10 a.m., Pavel Moravec wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21153/
> -----------------------------------------------------------
> 
> (Updated May 7, 2014, 10 a.m.)
> 
> 
> Review request for qpid, Gordon Sim and mick goulish.
> 
> 
> Bugs: QPID-5748
>     https://issues.apache.org/jira/browse/QPID-5748
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> Queue::purgeExpired method (to remove messages from all queues with TTL expired) currently
calls Message::hasExpired() method that calls AbsTime::now() i.e. ::clock_gettime for every
individual message with TTL set.
> 
> That system call is redundant to be called in that way. It is enough to call it once
before traversing the very first queue and use the value as an argument.
> 
> The patch is not ideal: I would rather see just one function like:
> 
> bool ExpiryPolicy::hasExpired(const Message& m, qpid::sys::AbsTime time = 0)
> {
>   if time==0 time=sys::AbsTime::now();
>   return m.getExpiration() < sys::AbsTime::now();
> }
> 
> but not sure what value from AbsTime to be used instead of 0 (note that such value should
be get in as short time as possible). And I havent played with AbsTime class and dont have
time now to do so :-/
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1592542 
>   /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1592542 
>   /trunk/qpid/cpp/src/qpid/broker/Message.h 1592542 
>   /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1592542 
>   /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1592542 
> 
> Diff: https://reviews.apache.org/r/21153/diff/
> 
> 
> Testing
> -------
> 
> Attached patch improved:
> - _traversing_ queues and _checking_ messages with TTL by approx. 10%
> - with purging _durable_ expired messages, performance improvement is just 1% (still
better than nothing)
> 
> No automated tests run so far (lack of time+HW).
> 
> 
> Thanks,
> 
> Pavel Moravec
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message