You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2013/05/24 16:16:42 UTC
Review Request: QPID-4886: Pass non-const reference to Message in
QueueObserver functions.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11378/
-----------------------------------------------------------
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
Re: Review Request: QPID-4886: Pass non-const reference to Message in
QueueObserver functions.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11378/#review21156
-----------------------------------------------------------
Ship it!
Ship It!
- Kenneth Giusti
On May 29, 2013, 2:36 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11378/
> -----------------------------------------------------------
>
> (Updated May 29, 2013, 2:36 p.m.)
>
>
> Review request for qpid, Gordon Sim and Kenneth Giusti.
>
>
> Description
> -------
>
> QPID-4886: Pass non-const reference to Message in QueueObserver functions.
>
> Based on discussion on https://reviews.apache.org/r/11378/ we will not modify
> QueueObserver to allow message modification as it violates the existing
> semantics. Instead this patch introduces a new class MessageInterceptor for that
> purpose.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/MessageInterceptor.h PRE-CREATION
> /trunk/qpid/cpp/src/qpid/broker/Queue.h 1487501
> /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1487501
>
> Diff: https://reviews.apache.org/r/11378/diff/
>
>
> Testing
> -------
>
> make test
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request: QPID-4886: Pass non-const reference to Message in
QueueObserver functions.
Posted by Alan Conway <ac...@redhat.com>.
> On May 29, 2013, 2:51 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/MessageInterceptor.h, line 42
> > <https://reviews.apache.org/r/11378/diff/2/?file=298102#file298102line42>
> >
> > I have a slight preference for 'publish()' rather than 'push()', i.e. modify a message before it is published (made available to subscribers).
> >
> > In the future there then may be another method in the interface named 'record()', for modifying a message before it is written to disk.
Agreed on both points. Thanks!
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11378/#review21155
-----------------------------------------------------------
On May 29, 2013, 2:36 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11378/
> -----------------------------------------------------------
>
> (Updated May 29, 2013, 2:36 p.m.)
>
>
> Review request for qpid, Gordon Sim and Kenneth Giusti.
>
>
> Description
> -------
>
> QPID-4886: Pass non-const reference to Message in QueueObserver functions.
>
> Based on discussion on https://reviews.apache.org/r/11378/ we will not modify
> QueueObserver to allow message modification as it violates the existing
> semantics. Instead this patch introduces a new class MessageInterceptor for that
> purpose.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/MessageInterceptor.h PRE-CREATION
> /trunk/qpid/cpp/src/qpid/broker/Queue.h 1487501
> /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1487501
>
> Diff: https://reviews.apache.org/r/11378/diff/
>
>
> Testing
> -------
>
> make test
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request: QPID-4886: Pass non-const reference to Message in
QueueObserver functions.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11378/#review21155
-----------------------------------------------------------
Ship it!
/trunk/qpid/cpp/src/qpid/broker/MessageInterceptor.h
<https://reviews.apache.org/r/11378/#comment43927>
I have a slight preference for 'publish()' rather than 'push()', i.e. modify a message before it is published (made available to subscribers).
In the future there then may be another method in the interface named 'record()', for modifying a message before it is written to disk.
- Gordon Sim
On May 29, 2013, 2:36 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11378/
> -----------------------------------------------------------
>
> (Updated May 29, 2013, 2:36 p.m.)
>
>
> Review request for qpid, Gordon Sim and Kenneth Giusti.
>
>
> Description
> -------
>
> QPID-4886: Pass non-const reference to Message in QueueObserver functions.
>
> Based on discussion on https://reviews.apache.org/r/11378/ we will not modify
> QueueObserver to allow message modification as it violates the existing
> semantics. Instead this patch introduces a new class MessageInterceptor for that
> purpose.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/MessageInterceptor.h PRE-CREATION
> /trunk/qpid/cpp/src/qpid/broker/Queue.h 1487501
> /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1487501
>
> Diff: https://reviews.apache.org/r/11378/diff/
>
>
> Testing
> -------
>
> make test
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request: QPID-4886: Pass non-const reference to Message in
QueueObserver functions.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11378/
-----------------------------------------------------------
(Updated May 29, 2013, 2:36 p.m.)
Review request for qpid, Gordon Sim and Kenneth Giusti.
Changes
-------
Instead of modifying QueueObserver, add a new class MessageInterceptor.
Description (updated)
-------
QPID-4886: Pass non-const reference to Message in QueueObserver functions.
Based on discussion on https://reviews.apache.org/r/11378/ we will not modify
QueueObserver to allow message modification as it violates the existing
semantics. Instead this patch introduces a new class MessageInterceptor for that
purpose.
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/MessageInterceptor.h PRE-CREATION
/trunk/qpid/cpp/src/qpid/broker/Queue.h 1487501
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1487501
Diff: https://reviews.apache.org/r/11378/diff/
Testing
-------
make test
Thanks,
Alan Conway
Re: Review Request: QPID-4886: Pass non-const reference to Message in
QueueObserver functions.
Posted by Gordon Sim <gs...@redhat.com>.
> 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
>
>
Re: Review Request: QPID-4886: Pass non-const reference to Message in
QueueObserver functions.
Posted by Kenneth Giusti <kg...@apache.org>.
> 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?
+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.
- Kenneth
-----------------------------------------------------------
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
>
>
Re: Review Request: QPID-4886: Pass non-const reference to Message in
QueueObserver functions.
Posted by Alan Conway <ac...@redhat.com>.
> 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
>
>
Re: Review Request: QPID-4886: Pass non-const reference to Message in
QueueObserver functions.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
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
>
>