----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21153/#review43955 -----------------------------------------------------------
Looks good to me! Cleans things up nicely. /trunk/qpid/cpp/src/qpid/broker/Message.h <https://reviews.apache.org/r/21153/#comment78215> Rather than have a hasExpired() method that takes the tie to compare against, it might be simpler to remove this method entirely and just use getExpiration() which can then be compared against whatever value is desired. - Gordon Sim On May 26, 2014, 12:14 p.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21153/ > ----------------------------------------------------------- > > (Updated May 26, 2014, 12:14 p.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/CMakeLists.txt 1597555 > /trunk/qpid/cpp/src/qpid/broker/Broker.h 1597555 > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1597555 > /trunk/qpid/cpp/src/qpid/broker/Message.h 1597555 > /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1597555 > /trunk/qpid/cpp/src/qpid/broker/PagedQueue.h 1597555 > /trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp 1597555 > /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1597555 > /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1597555 > /trunk/qpid/cpp/src/qpid/broker/RecoverableMessage.h 1597555 > /trunk/qpid/cpp/src/qpid/broker/RecoverableMessageImpl.h 1597555 > /trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1597555 > /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1597555 > /trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.h 1597555 > /trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.cpp 1597555 > /trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.cpp 1597555 > /trunk/qpid/cpp/src/qpid/linearstore/MessageStoreImpl.cpp 1597555 > /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1597555 > /trunk/qpid/cpp/src/tests/MessageUtils.h 1597555 > /trunk/qpid/cpp/src/tests/QueueTest.cpp 1597555 > > 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 > >
