> 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. > > Gordon Sim wrote: > 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? > > Kenneth Giusti wrote: > +1 something like Gordon's suggestion - if possible. The notion that an > Observer can actually modify in addition to simply observing isn't intuitive > and overloads the original intent more than I think it should.
Agreed, I'll post an updated approach. - Alan ----------------------------------------------------------- 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 > >
