> On May 24, 2013, 2:35 p.m., Gordon Sim wrote: > > 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.
Perhaps a better solution would be to have a new callback for things outside the queue that wish to modify the message. This can be done at specific points (before writing to disk and before making it available to consumers) that are easier to reason about with regard to concurrent access. What do you think? Would that work for your case? - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11378/#review20996 ----------------------------------------------------------- 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 > >
