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 2012/01/20 18:47:07 UTC
Review Request: QPID-3603: HA backup rejects client connections.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3570/
-----------------------------------------------------------
Review request for qpid, Gordon Sim and Kenneth Giusti.
Summary
-------
This commit introduces a ConnectionObserver, needed by the HA code to get notified of connections opening.
I also took the LinkRegistry::notify* calls out of Connection and refactored LinkRegistry to use a ConnectionObserver.
Do the ConnectionObserver changes this look good to put on trunk?
Diffs
-----
/branches/qpid-3603-2/qpid/cpp/src/Makefile.am 1233690
/branches/qpid-3603-2/qpid/cpp/src/ha.mk 1233690
/branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Broker.h 1233690
/branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Connection.cpp 1233690
/branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObserver.h PRE-CREATION
/branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObservers.h PRE-CREATION
/branches/qpid-3603-2/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1233690
/branches/qpid-3603-2/qpid/cpp/src/qpid/ha/ConnectionExcluder.h PRE-CREATION
/branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaBroker.h 1233690
/branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaBroker.cpp 1233690
/branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaPlugin.cpp 1233690
/branches/qpid-3603-2/qpid/cpp/src/qpid/ha/Settings.h 1233690
/branches/qpid-3603-2/qpid/cpp/src/tests/ha_tests.py 1233690
Diff: https://reviews.apache.org/r/3570/diff
Testing
-------
Thanks,
Alan
Re: Review Request: QPID-3603: HA backup rejects client connections.
Posted by Alan Conway <ac...@redhat.com>.
> On 2012-01-20 19:14:44, Andrew Stitcher wrote:
> > /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObservers.h, line 34
> > <https://reviews.apache.org/r/3570/diff/1/?file=70162#file70162line34>
> >
> > I don't think that the class "ConnectionObservers" IS-A "ConnectionObserver" itself it seems to me that it has-a collection of "ConnectionObserver"s and delegates to them.
> >
> > I think you should completely remove the inheritance relationship.
The thinking is that a ConnectionObserver is a thing that is notified of connection events. The ConnectionObservers is a thing that is notified of events. Its implementation is to delegate to a collection of other ConnectionObservers but that's of no concern to the caller.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3570/#review4503
-----------------------------------------------------------
On 2012-01-20 17:47:07, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3570/
> -----------------------------------------------------------
>
> (Updated 2012-01-20 17:47:07)
>
>
> Review request for qpid, Gordon Sim and Kenneth Giusti.
>
>
> Summary
> -------
>
>
> This commit introduces a ConnectionObserver, needed by the HA code to get notified of connections opening.
> I also took the LinkRegistry::notify* calls out of Connection and refactored LinkRegistry to use a ConnectionObserver.
>
> Do the ConnectionObserver changes this look good to put on trunk?
>
>
> Diffs
> -----
>
> /branches/qpid-3603-2/qpid/cpp/src/Makefile.am 1233690
> /branches/qpid-3603-2/qpid/cpp/src/ha.mk 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Broker.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Connection.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObserver.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObservers.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/ConnectionExcluder.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaBroker.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaBroker.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaPlugin.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/Settings.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/tests/ha_tests.py 1233690
>
> Diff: https://reviews.apache.org/r/3570/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: QPID-3603: HA backup rejects client connections.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3570/#review4503
-----------------------------------------------------------
/branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObservers.h
<https://reviews.apache.org/r/3570/#comment10094>
I don't think that the class "ConnectionObservers" IS-A "ConnectionObserver" itself it seems to me that it has-a collection of "ConnectionObserver"s and delegates to them.
I think you should completely remove the inheritance relationship.
- Andrew
On 2012-01-20 17:47:07, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3570/
> -----------------------------------------------------------
>
> (Updated 2012-01-20 17:47:07)
>
>
> Review request for qpid, Gordon Sim and Kenneth Giusti.
>
>
> Summary
> -------
>
>
> This commit introduces a ConnectionObserver, needed by the HA code to get notified of connections opening.
> I also took the LinkRegistry::notify* calls out of Connection and refactored LinkRegistry to use a ConnectionObserver.
>
> Do the ConnectionObserver changes this look good to put on trunk?
>
>
> Diffs
> -----
>
> /branches/qpid-3603-2/qpid/cpp/src/Makefile.am 1233690
> /branches/qpid-3603-2/qpid/cpp/src/ha.mk 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Broker.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Connection.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObserver.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObservers.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/ConnectionExcluder.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaBroker.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaBroker.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaPlugin.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/Settings.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/tests/ha_tests.py 1233690
>
> Diff: https://reviews.apache.org/r/3570/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: QPID-3603: HA backup rejects client connections.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3570/#review4530
-----------------------------------------------------------
Ship it!
Looks good. Could ConnectionOberver be merged with Connection::ErrorListener? There seems to be a least a hint of overlap.
- Gordon
On 2012-01-20 17:47:07, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3570/
> -----------------------------------------------------------
>
> (Updated 2012-01-20 17:47:07)
>
>
> Review request for qpid, Gordon Sim and Kenneth Giusti.
>
>
> Summary
> -------
>
>
> This commit introduces a ConnectionObserver, needed by the HA code to get notified of connections opening.
> I also took the LinkRegistry::notify* calls out of Connection and refactored LinkRegistry to use a ConnectionObserver.
>
> Do the ConnectionObserver changes this look good to put on trunk?
>
>
> Diffs
> -----
>
> /branches/qpid-3603-2/qpid/cpp/src/Makefile.am 1233690
> /branches/qpid-3603-2/qpid/cpp/src/ha.mk 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Broker.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Connection.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObserver.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObservers.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/ConnectionExcluder.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaBroker.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaBroker.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaPlugin.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/Settings.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/tests/ha_tests.py 1233690
>
> Diff: https://reviews.apache.org/r/3570/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: QPID-3603: HA backup rejects client connections.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3570/#review4504
-----------------------------------------------------------
Ship it!
I like it - cleans up the hacks in the LinkRegistry nicely.
/branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Connection.cpp
<https://reviews.apache.org/r/3570/#comment10097>
Alan - can you move this call to the end of the method? Turns out Links will need to access the mgmt OID of the connection, which isn't set up until after the next line.
/branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Connection.cpp
<https://reviews.apache.org/r/3570/#comment10098>
The reverse here: probably should move this call *before* the mgmtObject is destroyed.
/branches/qpid-3603-2/qpid/cpp/src/qpid/broker/LinkRegistry.cpp
<https://reviews.apache.org/r/3570/#comment10095>
+1 - would it now be possible to make these LinkRegistry::notify* methods private to the LinkRegistry - that would make me happy!
- Kenneth
On 2012-01-20 17:47:07, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3570/
> -----------------------------------------------------------
>
> (Updated 2012-01-20 17:47:07)
>
>
> Review request for qpid, Gordon Sim and Kenneth Giusti.
>
>
> Summary
> -------
>
>
> This commit introduces a ConnectionObserver, needed by the HA code to get notified of connections opening.
> I also took the LinkRegistry::notify* calls out of Connection and refactored LinkRegistry to use a ConnectionObserver.
>
> Do the ConnectionObserver changes this look good to put on trunk?
>
>
> Diffs
> -----
>
> /branches/qpid-3603-2/qpid/cpp/src/Makefile.am 1233690
> /branches/qpid-3603-2/qpid/cpp/src/ha.mk 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Broker.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/Connection.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObserver.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/ConnectionObservers.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/ConnectionExcluder.h PRE-CREATION
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaBroker.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaBroker.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/HaPlugin.cpp 1233690
> /branches/qpid-3603-2/qpid/cpp/src/qpid/ha/Settings.h 1233690
> /branches/qpid-3603-2/qpid/cpp/src/tests/ha_tests.py 1233690
>
> Diff: https://reviews.apache.org/r/3570/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>