> On 2011-08-08 10:11:17, Gordon Sim wrote: > > /branches/qpid-3346/qpid/cpp/src/qpid/broker/LegacyLVQ.cpp, line 39 > > <https://reviews.apache.org/r/1312/diff/1/?file=30946#file30946line39> > > > > The purpose of the check is to ensure that an acquire attempt for a > > message that has since been replaced, does not acquire the message that has > > replaced it instead. > > > > I believe it is still necessary, though I concede the form is ugly and > > unintuitive. > > Kenneth Giusti wrote: > Agreed that it is necessary for LVQ, but the abstract api for > 'Messages::remove' can't explicitly enforce that the caller supply the actual > target msg, just the position. Given that, it's pretty easy for the caller > to get this wrong without discovering it (which actually happened to me - > thank goodness for unit tests :). > > And the "move message/purge message" management operations probably could > use a remove method that doesn't necessitate a preceding message find(). > > I'm thinking of two different message-removal use-cases: > > bool Messages::remove( position, QueuedMessage *result) - remove msg at > position, return true and set result if found > bool Messages::remove( const QueuedMessage& target ) - remove target msg, > return true if msg was found. This variant could be used by 'acquire' or > other paths that have already retrieved the QueuedMessage. > > Although, defining a iterator for the Messages class probably renders > this whole issue moot.
Yes, I think you are right that the confusion comes from trying to use the same codepath for these two cases and separating them would be clearer. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1312/#review1317 ----------------------------------------------------------- On 2011-08-05 20:46:15, Kenneth Giusti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1312/ > ----------------------------------------------------------- > > (Updated 2011-08-05 20:46:15) > > > Review request for qpid. > > > Summary > ------- > > Some initial refactoring of Queue/Consumer interface to allow for message > grouping support. Very preliminary. > > > This addresses bug qpid-3346. > https://issues.apache.org/jira/browse/qpid-3346 > > > Diffs > ----- > > /branches/qpid-3346/qpid/cpp/src/qpid/broker/Consumer.h 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/DeliveryRecord.h 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/LegacyLVQ.cpp 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/Queue.h 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/Queue.cpp 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueEvents.cpp 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueObserver.h 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueuePolicy.cpp 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/SemanticState.h 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/SemanticState.cpp 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1144324 > /branches/qpid-3346/qpid/cpp/src/qpid/cluster/Connection.cpp 1144324 > /branches/qpid-3346/qpid/cpp/src/tests/QueueTest.cpp 1144324 > > Diff: https://reviews.apache.org/r/1312/diff > > > Testing > ------- > > minimal - passes unit tests on linux. > > > Thanks, > > Kenneth > >
