----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11378/#review20996 -----------------------------------------------------------
This makes me a little nervous, specifically around modification while the message is being concurrently read by some other thread. At present the observers are only called from queue while lock is held but can we rule out any other concurrent access? Having the queue be the only place where the message can be modified (modifications elsewhere requiring a copy) seems like a good practice. With regard to annotations, the observeEnqueue() method is only called after the message has been written to disk, so any modifications at that point would certainly not be durable. - Gordon Sim On May 24, 2013, 2:16 p.m., Alan Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11378/ > ----------------------------------------------------------- > > (Updated May 24, 2013, 2:16 p.m.) > > > Review request for qpid, Gordon Sim and Kenneth Giusti. > > > Description > ------- > > QPID-4886: Pass non-const reference to Message in QueueObserver functions. > > The HA plugin needs to modify a message in QueueObserver::enqueued to attach > a HA ID to it. Other plugins may need to similarly annotate messages at > QueueObserver call points. > Need to remove the "const" qualifier for Message arguments to QueueObserver > methods to enable this. > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/broker/MessageGroupManager.h 1486017 > /trunk/qpid/cpp/src/qpid/broker/MessageGroupManager.cpp 1486017 > /trunk/qpid/cpp/src/qpid/broker/Queue.h 1486017 > /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1486017 > /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1486017 > /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1486017 > /trunk/qpid/cpp/src/qpid/broker/QueueObserver.h 1486017 > /trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1486017 > /trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1486017 > /trunk/qpid/cpp/src/qpid/ha/QueueGuard.h 1486017 > /trunk/qpid/cpp/src/qpid/ha/QueueGuard.cpp 1486017 > /trunk/qpid/cpp/src/qpid/ha/QueueReplicator.cpp 1486017 > > Diff: https://reviews.apache.org/r/11378/diff/ > > > Testing > ------- > > make test > > > Thanks, > > Alan Conway > >
