You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Gordon Sim <gs...@redhat.com> on 2012/11/01 09:41:53 UTC
Re: Review Request: QPID-4394: HA QMF events out of order
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7814/#review12991
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/management/ManagementAgent.h
<https://reviews.apache.org/r/7814/#comment27969>
Remind me why we can't push the ManagementEvent object itself onto the PollableQueue, and move all the logic of raiseEvent onto another thread?
- Gordon Sim
On Oct. 31, 2012, 8:30 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7814/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2012, 8:30 p.m.)
>
>
> Review request for qpid, Gordon Sim, Kenneth Giusti, Andy Goldstein, and Jason Dillaman.
>
>
> Description
> -------
>
> QPID-4394: HA QMF events out of order
>
> QMF create/delete events for auto-delete queues can be generated out-of-order
> because they are not regulated by any lock. This creates problems for HA
> replication. This commit does 2 things:
>
> 1. Dispatch QMF events via a pollable queue. Events are encoded in the thread
> calling raiseEvent then put on a PollableQueue to be dispatched in a separate thread.
> This allows us to move the raiseEvent calls inside registry locks to ensure
> they are ordered.
>
> 2. Move queue create and delete raiseEvent calls inside the queue registry
> lock so they are executed in order.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1402462
> /trunk/qpid/cpp/src/qpid/broker/QueueRegistry.h 1402462
> /trunk/qpid/cpp/src/qpid/broker/QueueRegistry.cpp 1402462
> /trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1402462
> /trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1402462
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1402462
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1402462
> /trunk/qpid/cpp/src/tests/ha_tests.py 1402462
>
> Diff: https://reviews.apache.org/r/7814/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request: QPID-4394: HA QMF events out of order
Posted by Gordon Sim <gs...@redhat.com>.
> On Nov. 1, 2012, 8:41 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h, line 347
> > <https://reviews.apache.org/r/7814/diff/1/?file=183515#file183515line347>
> >
> > Remind me why we can't push the ManagementEvent object itself onto the PollableQueue, and move all the logic of raiseEvent onto another thread?
>
> Alan Conway wrote:
> ManagementEvent subclasses don't contain any data, they just have reference members plus they are polymorphic so they can't be passed by value. To make this work you need to change the ManangementObject subclasses to contain values, and pass them by shared_ptr. I started to do it but backed off because it breaks the ABI for the store and possibly other plugins that aren't in the qpid tree (windows store?) It is *much* cleaner though. Do you think it's worth the compatibility breakage?
Ah yes, of course. I think what you have is good enough for now. We can consider a change if we get to revisiting the management code more generally.
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7814/#review12991
-----------------------------------------------------------
On Oct. 31, 2012, 8:30 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7814/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2012, 8:30 p.m.)
>
>
> Review request for qpid, Gordon Sim, Kenneth Giusti, Andy Goldstein, and Jason Dillaman.
>
>
> Description
> -------
>
> QPID-4394: HA QMF events out of order
>
> QMF create/delete events for auto-delete queues can be generated out-of-order
> because they are not regulated by any lock. This creates problems for HA
> replication. This commit does 2 things:
>
> 1. Dispatch QMF events via a pollable queue. Events are encoded in the thread
> calling raiseEvent then put on a PollableQueue to be dispatched in a separate thread.
> This allows us to move the raiseEvent calls inside registry locks to ensure
> they are ordered.
>
> 2. Move queue create and delete raiseEvent calls inside the queue registry
> lock so they are executed in order.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1402462
> /trunk/qpid/cpp/src/qpid/broker/QueueRegistry.h 1402462
> /trunk/qpid/cpp/src/qpid/broker/QueueRegistry.cpp 1402462
> /trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1402462
> /trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1402462
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1402462
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1402462
> /trunk/qpid/cpp/src/tests/ha_tests.py 1402462
>
> Diff: https://reviews.apache.org/r/7814/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request: QPID-4394: HA QMF events out of order
Posted by Alan Conway <ac...@redhat.com>.
> On Nov. 1, 2012, 8:41 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h, line 347
> > <https://reviews.apache.org/r/7814/diff/1/?file=183515#file183515line347>
> >
> > Remind me why we can't push the ManagementEvent object itself onto the PollableQueue, and move all the logic of raiseEvent onto another thread?
ManagementEvent subclasses don't contain any data, they just have reference members plus they are polymorphic so they can't be passed by value. To make this work you need to change the ManangementObject subclasses to contain values, and pass them by shared_ptr. I started to do it but backed off because it breaks the ABI for the store and possibly other plugins that aren't in the qpid tree (windows store?) It is *much* cleaner though. Do you think it's worth the compatibility breakage?
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7814/#review12991
-----------------------------------------------------------
On Oct. 31, 2012, 8:30 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7814/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2012, 8:30 p.m.)
>
>
> Review request for qpid, Gordon Sim, Kenneth Giusti, Andy Goldstein, and Jason Dillaman.
>
>
> Description
> -------
>
> QPID-4394: HA QMF events out of order
>
> QMF create/delete events for auto-delete queues can be generated out-of-order
> because they are not regulated by any lock. This creates problems for HA
> replication. This commit does 2 things:
>
> 1. Dispatch QMF events via a pollable queue. Events are encoded in the thread
> calling raiseEvent then put on a PollableQueue to be dispatched in a separate thread.
> This allows us to move the raiseEvent calls inside registry locks to ensure
> they are ordered.
>
> 2. Move queue create and delete raiseEvent calls inside the queue registry
> lock so they are executed in order.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1402462
> /trunk/qpid/cpp/src/qpid/broker/QueueRegistry.h 1402462
> /trunk/qpid/cpp/src/qpid/broker/QueueRegistry.cpp 1402462
> /trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1402462
> /trunk/qpid/cpp/src/qpid/ha/HaBroker.cpp 1402462
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1402462
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1402462
> /trunk/qpid/cpp/src/tests/ha_tests.py 1402462
>
> Diff: https://reviews.apache.org/r/7814/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan Conway
>
>