You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Andrew Stitcher <as...@apache.org> on 2013/06/04 19:09:09 UTC
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/
-----------------------------------------------------------
(Updated June 4, 2013, 5:09 p.m.)
Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
Changes
-------
I've finished constructing the review:
Each of the changes to the diff represents an individual and usually fairly small change (except the last which is a fix up for the autotools build)
I'm mostly interested in a sanity check because most of the changes are straightforward, but in the end quite a lot of code is changed. The change merging broker::Connection and broker::ConnectionState may be controversial as it's a larger than the others.
In any case all comments welcome.
Description
-------
A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
Change 1. - Remove unused members in Connector interface.
This addresses bug QPID-4905.
https://issues.apache.org/jira/browse/QPID-4905
Diffs
-----
/trunk/qpid/cpp/src/CMakeLists.txt 1489458
/trunk/qpid/cpp/src/Makefile.am 1489458
/trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
/trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
/trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
/trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
/trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
/trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
/trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
/trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
/trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
/trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
/trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
/trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
/trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
/trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
/trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
/trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
/trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
/trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
/trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
/trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
/trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
/trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
/trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
/trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
/trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
Diff: https://reviews.apache.org/r/11628/diff/
Testing
-------
cmake: make ; make test.
autotools: make ; make check
Thanks,
Andrew Stitcher
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/#review21665
-----------------------------------------------------------
I've now committed everything except the Connection and ConnectionState merge. I had an ides that might make this a bit more decoupled from the Broker object, and will see if that pans out.
If it doesn't, I may still check the change in, as I don't think it actually makes any further work harder, even if Gordon doesn't thin it is actually an improvement.
- Andrew Stitcher
On June 6, 2013, 3:40 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11628/
> -----------------------------------------------------------
>
> (Updated June 6, 2013, 3:40 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
>
>
> Description
> -------
>
> A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
>
> Change 1. - Remove unused members in Connector interface.
>
>
> This addresses bug QPID-4905.
> https://issues.apache.org/jira/browse/QPID-4905
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1489458
> /trunk/qpid/cpp/src/Makefile.am 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
>
> Diff: https://reviews.apache.org/r/11628/diff/
>
>
> Testing
> -------
>
> cmake: make ; make test.
> autotools: make ; make check
>
> Simple heartbeat test:
> - Run qpid-receive -f against qpidd;
> - Stop qpidd - see that qpid-receive disconnects after heartbeat interval
> - Restart qpidd
> - Start new qpid-receive -f
> - Stop qpid-receive - see that qpidd disconnects after heartbeat interval
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/#review21576
-----------------------------------------------------------
Ship it!
Changes all make sense to me. I don't have a strong opinion on the debate about adding a protocol independent interface as part of this vs a step after this.
- Alan Conway
On June 6, 2013, 3:40 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11628/
> -----------------------------------------------------------
>
> (Updated June 6, 2013, 3:40 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
>
>
> Description
> -------
>
> A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
>
> Change 1. - Remove unused members in Connector interface.
>
>
> This addresses bug QPID-4905.
> https://issues.apache.org/jira/browse/QPID-4905
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1489458
> /trunk/qpid/cpp/src/Makefile.am 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
>
> Diff: https://reviews.apache.org/r/11628/diff/
>
>
> Testing
> -------
>
> cmake: make ; make test.
> autotools: make ; make check
>
> Simple heartbeat test:
> - Run qpid-receive -f against qpidd;
> - Stop qpidd - see that qpid-receive disconnects after heartbeat interval
> - Restart qpidd
> - Start new qpid-receive -f
> - Stop qpid-receive - see that qpidd disconnects after heartbeat interval
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/#review21581
-----------------------------------------------------------
Ship it!
Ship It!
- Kenneth Giusti
On June 6, 2013, 3:40 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11628/
> -----------------------------------------------------------
>
> (Updated June 6, 2013, 3:40 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
>
>
> Description
> -------
>
> A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
>
> Change 1. - Remove unused members in Connector interface.
>
>
> This addresses bug QPID-4905.
> https://issues.apache.org/jira/browse/QPID-4905
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1489458
> /trunk/qpid/cpp/src/Makefile.am 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
>
> Diff: https://reviews.apache.org/r/11628/diff/
>
>
> Testing
> -------
>
> cmake: make ; make test.
> autotools: make ; make check
>
> Simple heartbeat test:
> - Run qpid-receive -f against qpidd;
> - Stop qpidd - see that qpid-receive disconnects after heartbeat interval
> - Restart qpidd
> - Start new qpid-receive -f
> - Stop qpid-receive - see that qpidd disconnects after heartbeat interval
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/
-----------------------------------------------------------
(Updated June 6, 2013, 3:40 p.m.)
Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
Changes
-------
Simple heartbeat testing done
Description
-------
A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
Change 1. - Remove unused members in Connector interface.
This addresses bug QPID-4905.
https://issues.apache.org/jira/browse/QPID-4905
Diffs
-----
/trunk/qpid/cpp/src/CMakeLists.txt 1489458
/trunk/qpid/cpp/src/Makefile.am 1489458
/trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
/trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
/trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
/trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
/trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
/trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
/trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
/trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
/trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
/trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
/trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
/trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
/trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
/trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
/trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
/trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
/trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
/trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
/trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
/trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
/trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
/trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
/trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
/trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
/trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
/trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
Diff: https://reviews.apache.org/r/11628/diff/
Testing (updated)
-------
cmake: make ; make test.
autotools: make ; make check
Simple heartbeat test:
- Run qpid-receive -f against qpidd;
- Stop qpidd - see that qpid-receive disconnects after heartbeat interval
- Restart qpidd
- Start new qpid-receive -f
- Stop qpid-receive - see that qpidd disconnects after heartbeat interval
Thanks,
Andrew Stitcher
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Gordon Sim <gs...@redhat.com>.
> On June 6, 2013, 12:18 p.m., Gordon Sim wrote:
> >
Also... there are no tests for heartbeats and since this touches those codepaths, have you verified it still works?
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/#review21516
-----------------------------------------------------------
On June 4, 2013, 5:09 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11628/
> -----------------------------------------------------------
>
> (Updated June 4, 2013, 5:09 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
>
>
> Description
> -------
>
> A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
>
> Change 1. - Remove unused members in Connector interface.
>
>
> This addresses bug QPID-4905.
> https://issues.apache.org/jira/browse/QPID-4905
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1489458
> /trunk/qpid/cpp/src/Makefile.am 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
>
> Diff: https://reviews.apache.org/r/11628/diff/
>
>
> Testing
> -------
>
> cmake: make ; make test.
> autotools: make ; make check
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Gordon Sim <gs...@redhat.com>.
> On June 6, 2013, 12:18 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp, line 712
> > <https://reviews.apache.org/r/11628/diff/9/?file=300660#file300660line712>
> >
> > The ConnectionState/Connection split is certainly seems a little arbitrary at present, but requiring the broker connection object in places like this will make it even harder to get AMQP 1.0 working with older features (e.g. get QMF over 1.0 in this case). Personally I'd prefer paring down the ConnectionState interface to the minimal interface needed from protocol independent contexts (and use the Connection impl itself within 0-10 specific contexts).
>
> Andrew Stitcher wrote:
> I agree that we need a neat protocol independent broker connection object, mostly to hold the management state. I was not attempting to do that in this change, but I think it would make a good next step.
>
> Essentially I would propose leaving what i called broker::Connection after my change as *that* object and push all the protocol dependent connection stuff elsewhere (we already have amqp_0_10::Connection which is probably a good place for it - at least as far as name is concerned).
>
> Having said that, there is nothing in the reformed Connection class that is actually protocol specific as far as I can see, it gets in the frame flow and that might be an issue for the 1.0 implementation because it sidesteps this aspect of the existing broker code.
>
> Gordon Sim wrote:
> It *is* protocol specific. As well as the links to qpid::framing and AMQFrame (all 0-10 specific) it is tied directly to ConnectionHandler and SessionHandler which are also 0-10 specific.
>
> Andrew Stitcher wrote:
> I retract that last comment, it is 0_10 specific. Those aspects should be extracted to a 0_10 specific place.
I think it would be much simpler to instead pull out the non 0-10 specific aspects (starting only with those actually needed from some other context) as that is a much, much smaller set. Vast amounts of the qpid::broker namespace are still 0-10 specific. Moving those classes (including this one) to another namespace would certainly make that clearer.
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/#review21516
-----------------------------------------------------------
On June 4, 2013, 5:09 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11628/
> -----------------------------------------------------------
>
> (Updated June 4, 2013, 5:09 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
>
>
> Description
> -------
>
> A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
>
> Change 1. - Remove unused members in Connector interface.
>
>
> This addresses bug QPID-4905.
> https://issues.apache.org/jira/browse/QPID-4905
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1489458
> /trunk/qpid/cpp/src/Makefile.am 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
>
> Diff: https://reviews.apache.org/r/11628/diff/
>
>
> Testing
> -------
>
> cmake: make ; make test.
> autotools: make ; make check
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Andrew Stitcher <as...@apache.org>.
> On June 6, 2013, 12:18 p.m., Gordon Sim wrote:
> >
>
> Gordon Sim wrote:
> Also... there are no tests for heartbeats and since this touches those codepaths, have you verified it still works?
Good point - I did not verify it specifically after these changes. I will check it.
However I'd say that these changes only rename something in the heartbeat path.
The removed idleOut() cannot have broken heartbeats itself as verifiably it is never called anywhere - the only hits for "git grep idleOut" are definitions (and nearly all empty) so if heartbeats are broken because this is never called then it was broken before.
> On June 6, 2013, 12:18 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp, line 712
> > <https://reviews.apache.org/r/11628/diff/9/?file=300660#file300660line712>
> >
> > The ConnectionState/Connection split is certainly seems a little arbitrary at present, but requiring the broker connection object in places like this will make it even harder to get AMQP 1.0 working with older features (e.g. get QMF over 1.0 in this case). Personally I'd prefer paring down the ConnectionState interface to the minimal interface needed from protocol independent contexts (and use the Connection impl itself within 0-10 specific contexts).
I agree that we need a neat protocol independent broker connection object, mostly to hold the management state. I was not attempting to do that in this change, but I think it would make a good next step.
Essentially I would propose leaving what i called broker::Connection after my change as *that* object and push all the protocol dependent connection stuff elsewhere (we already have amqp_0_10::Connection which is probably a good place for it - at least as far as name is concerned).
Having said that, there is nothing in the reformed Connection class that is actually protocol specific as far as I can see, it gets in the frame flow and that might be an issue for the 1.0 implementation because it sidesteps this aspect of the existing broker code.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/#review21516
-----------------------------------------------------------
On June 4, 2013, 5:09 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11628/
> -----------------------------------------------------------
>
> (Updated June 4, 2013, 5:09 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
>
>
> Description
> -------
>
> A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
>
> Change 1. - Remove unused members in Connector interface.
>
>
> This addresses bug QPID-4905.
> https://issues.apache.org/jira/browse/QPID-4905
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1489458
> /trunk/qpid/cpp/src/Makefile.am 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
>
> Diff: https://reviews.apache.org/r/11628/diff/
>
>
> Testing
> -------
>
> cmake: make ; make test.
> autotools: make ; make check
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Andrew Stitcher <as...@apache.org>.
> On June 6, 2013, 12:18 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp, line 712
> > <https://reviews.apache.org/r/11628/diff/9/?file=300660#file300660line712>
> >
> > The ConnectionState/Connection split is certainly seems a little arbitrary at present, but requiring the broker connection object in places like this will make it even harder to get AMQP 1.0 working with older features (e.g. get QMF over 1.0 in this case). Personally I'd prefer paring down the ConnectionState interface to the minimal interface needed from protocol independent contexts (and use the Connection impl itself within 0-10 specific contexts).
>
> Andrew Stitcher wrote:
> I agree that we need a neat protocol independent broker connection object, mostly to hold the management state. I was not attempting to do that in this change, but I think it would make a good next step.
>
> Essentially I would propose leaving what i called broker::Connection after my change as *that* object and push all the protocol dependent connection stuff elsewhere (we already have amqp_0_10::Connection which is probably a good place for it - at least as far as name is concerned).
>
> Having said that, there is nothing in the reformed Connection class that is actually protocol specific as far as I can see, it gets in the frame flow and that might be an issue for the 1.0 implementation because it sidesteps this aspect of the existing broker code.
>
> Gordon Sim wrote:
> It *is* protocol specific. As well as the links to qpid::framing and AMQFrame (all 0-10 specific) it is tied directly to ConnectionHandler and SessionHandler which are also 0-10 specific.
I retract that last comment, it is 0_10 specific. Those aspects should be extracted to a 0_10 specific place.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/#review21516
-----------------------------------------------------------
On June 4, 2013, 5:09 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11628/
> -----------------------------------------------------------
>
> (Updated June 4, 2013, 5:09 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
>
>
> Description
> -------
>
> A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
>
> Change 1. - Remove unused members in Connector interface.
>
>
> This addresses bug QPID-4905.
> https://issues.apache.org/jira/browse/QPID-4905
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1489458
> /trunk/qpid/cpp/src/Makefile.am 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
>
> Diff: https://reviews.apache.org/r/11628/diff/
>
>
> Testing
> -------
>
> cmake: make ; make test.
> autotools: make ; make check
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Gordon Sim <gs...@redhat.com>.
> On June 6, 2013, 12:18 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp, line 712
> > <https://reviews.apache.org/r/11628/diff/9/?file=300660#file300660line712>
> >
> > The ConnectionState/Connection split is certainly seems a little arbitrary at present, but requiring the broker connection object in places like this will make it even harder to get AMQP 1.0 working with older features (e.g. get QMF over 1.0 in this case). Personally I'd prefer paring down the ConnectionState interface to the minimal interface needed from protocol independent contexts (and use the Connection impl itself within 0-10 specific contexts).
>
> Andrew Stitcher wrote:
> I agree that we need a neat protocol independent broker connection object, mostly to hold the management state. I was not attempting to do that in this change, but I think it would make a good next step.
>
> Essentially I would propose leaving what i called broker::Connection after my change as *that* object and push all the protocol dependent connection stuff elsewhere (we already have amqp_0_10::Connection which is probably a good place for it - at least as far as name is concerned).
>
> Having said that, there is nothing in the reformed Connection class that is actually protocol specific as far as I can see, it gets in the frame flow and that might be an issue for the 1.0 implementation because it sidesteps this aspect of the existing broker code.
>
> Gordon Sim wrote:
> It *is* protocol specific. As well as the links to qpid::framing and AMQFrame (all 0-10 specific) it is tied directly to ConnectionHandler and SessionHandler which are also 0-10 specific.
>
> Andrew Stitcher wrote:
> I retract that last comment, it is 0_10 specific. Those aspects should be extracted to a 0_10 specific place.
>
> Gordon Sim wrote:
> I think it would be much simpler to instead pull out the non 0-10 specific aspects (starting only with those actually needed from some other context) as that is a much, much smaller set. Vast amounts of the qpid::broker namespace are still 0-10 specific. Moving those classes (including this one) to another namespace would certainly make that clearer.
>
> Andrew Stitcher wrote:
> I'm not sure if we are disagreeing at this point - do you think that the merging makes this harder?
>
> As far as I can see what you are suggesting if applied to the current state means creating yet a third class (as currently both are protocol specific), or it means moving stuff around between 2 classes that are roughly analogous to what we have here with a clearer distinction.
>
> My idea would be to merge then split apart. I think you are suggesting shifting stuff from one class to the other (in both directions). Is this a fair summary? I don't think what I have done here stops a later split and I don't think it even makes it harder, but I understand you might have a different idea.
I think paring ConnectionState down to the minimal interface currently needed in protocol independent contexts (and then using Connection directly elsewhere) is a clearer step forward. (I don't think anything in Connection needs to be moved up into ConnectionState).
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/#review21516
-----------------------------------------------------------
On June 6, 2013, 3:40 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11628/
> -----------------------------------------------------------
>
> (Updated June 6, 2013, 3:40 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
>
>
> Description
> -------
>
> A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
>
> Change 1. - Remove unused members in Connector interface.
>
>
> This addresses bug QPID-4905.
> https://issues.apache.org/jira/browse/QPID-4905
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1489458
> /trunk/qpid/cpp/src/Makefile.am 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
>
> Diff: https://reviews.apache.org/r/11628/diff/
>
>
> Testing
> -------
>
> cmake: make ; make test.
> autotools: make ; make check
>
> Simple heartbeat test:
> - Run qpid-receive -f against qpidd;
> - Stop qpidd - see that qpid-receive disconnects after heartbeat interval
> - Restart qpidd
> - Start new qpid-receive -f
> - Stop qpid-receive - see that qpidd disconnects after heartbeat interval
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Andrew Stitcher <as...@apache.org>.
> On June 6, 2013, 12:18 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp, line 712
> > <https://reviews.apache.org/r/11628/diff/9/?file=300660#file300660line712>
> >
> > The ConnectionState/Connection split is certainly seems a little arbitrary at present, but requiring the broker connection object in places like this will make it even harder to get AMQP 1.0 working with older features (e.g. get QMF over 1.0 in this case). Personally I'd prefer paring down the ConnectionState interface to the minimal interface needed from protocol independent contexts (and use the Connection impl itself within 0-10 specific contexts).
>
> Andrew Stitcher wrote:
> I agree that we need a neat protocol independent broker connection object, mostly to hold the management state. I was not attempting to do that in this change, but I think it would make a good next step.
>
> Essentially I would propose leaving what i called broker::Connection after my change as *that* object and push all the protocol dependent connection stuff elsewhere (we already have amqp_0_10::Connection which is probably a good place for it - at least as far as name is concerned).
>
> Having said that, there is nothing in the reformed Connection class that is actually protocol specific as far as I can see, it gets in the frame flow and that might be an issue for the 1.0 implementation because it sidesteps this aspect of the existing broker code.
>
> Gordon Sim wrote:
> It *is* protocol specific. As well as the links to qpid::framing and AMQFrame (all 0-10 specific) it is tied directly to ConnectionHandler and SessionHandler which are also 0-10 specific.
>
> Andrew Stitcher wrote:
> I retract that last comment, it is 0_10 specific. Those aspects should be extracted to a 0_10 specific place.
>
> Gordon Sim wrote:
> I think it would be much simpler to instead pull out the non 0-10 specific aspects (starting only with those actually needed from some other context) as that is a much, much smaller set. Vast amounts of the qpid::broker namespace are still 0-10 specific. Moving those classes (including this one) to another namespace would certainly make that clearer.
I'm not sure if we are disagreeing at this point - do you think that the merging makes this harder?
As far as I can see what you are suggesting if applied to the current state means creating yet a third class (as currently both are protocol specific), or it means moving stuff around between 2 classes that are roughly analogous to what we have here with a clearer distinction.
My idea would be to merge then split apart. I think you are suggesting shifting stuff from one class to the other (in both directions). Is this a fair summary? I don't think what I have done here stops a later split and I don't think it even makes it harder, but I understand you might have a different idea.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/#review21516
-----------------------------------------------------------
On June 6, 2013, 3:40 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11628/
> -----------------------------------------------------------
>
> (Updated June 6, 2013, 3:40 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
>
>
> Description
> -------
>
> A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
>
> Change 1. - Remove unused members in Connector interface.
>
>
> This addresses bug QPID-4905.
> https://issues.apache.org/jira/browse/QPID-4905
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1489458
> /trunk/qpid/cpp/src/Makefile.am 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
>
> Diff: https://reviews.apache.org/r/11628/diff/
>
>
> Testing
> -------
>
> cmake: make ; make test.
> autotools: make ; make check
>
> Simple heartbeat test:
> - Run qpid-receive -f against qpidd;
> - Stop qpidd - see that qpid-receive disconnects after heartbeat interval
> - Restart qpidd
> - Start new qpid-receive -f
> - Stop qpid-receive - see that qpidd disconnects after heartbeat interval
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Gordon Sim <gs...@redhat.com>.
> On June 6, 2013, 12:18 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp, line 712
> > <https://reviews.apache.org/r/11628/diff/9/?file=300660#file300660line712>
> >
> > The ConnectionState/Connection split is certainly seems a little arbitrary at present, but requiring the broker connection object in places like this will make it even harder to get AMQP 1.0 working with older features (e.g. get QMF over 1.0 in this case). Personally I'd prefer paring down the ConnectionState interface to the minimal interface needed from protocol independent contexts (and use the Connection impl itself within 0-10 specific contexts).
>
> Andrew Stitcher wrote:
> I agree that we need a neat protocol independent broker connection object, mostly to hold the management state. I was not attempting to do that in this change, but I think it would make a good next step.
>
> Essentially I would propose leaving what i called broker::Connection after my change as *that* object and push all the protocol dependent connection stuff elsewhere (we already have amqp_0_10::Connection which is probably a good place for it - at least as far as name is concerned).
>
> Having said that, there is nothing in the reformed Connection class that is actually protocol specific as far as I can see, it gets in the frame flow and that might be an issue for the 1.0 implementation because it sidesteps this aspect of the existing broker code.
It *is* protocol specific. As well as the links to qpid::framing and AMQFrame (all 0-10 specific) it is tied directly to ConnectionHandler and SessionHandler which are also 0-10 specific.
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/#review21516
-----------------------------------------------------------
On June 4, 2013, 5:09 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11628/
> -----------------------------------------------------------
>
> (Updated June 4, 2013, 5:09 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
>
>
> Description
> -------
>
> A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
>
> Change 1. - Remove unused members in Connector interface.
>
>
> This addresses bug QPID-4905.
> https://issues.apache.org/jira/browse/QPID-4905
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1489458
> /trunk/qpid/cpp/src/Makefile.am 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
>
> Diff: https://reviews.apache.org/r/11628/diff/
>
>
> Testing
> -------
>
> cmake: make ; make test.
> autotools: make ; make check
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: C++ code spring cleaning - mostly removing unused stuff,
and tidying up some of the interfaces
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11628/#review21516
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp
<https://reviews.apache.org/r/11628/#comment44557>
The ConnectionState/Connection split is certainly seems a little arbitrary at present, but requiring the broker connection object in places like this will make it even harder to get AMQP 1.0 working with older features (e.g. get QMF over 1.0 in this case). Personally I'd prefer paring down the ConnectionState interface to the minimal interface needed from protocol independent contexts (and use the Connection impl itself within 0-10 specific contexts).
- Gordon Sim
On June 4, 2013, 5:09 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11628/
> -----------------------------------------------------------
>
> (Updated June 4, 2013, 5:09 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, Kenneth Giusti, and Ted Ross.
>
>
> Description
> -------
>
> A bunch of tidy ups to the C++ qpid code see the JIRA for a more detailed breakdown.
>
> Change 1. - Remove unused members in Connector interface.
>
>
> This addresses bug QPID-4905.
> https://issues.apache.org/jira/browse/QPID-4905
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1489458
> /trunk/qpid/cpp/src/Makefile.am 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/amqp_0_10/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/ConnectionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/HandlerImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1489458
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.h 1489458
> /trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/Connector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1489458
> /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1489458
> /trunk/qpid/cpp/src/qpid/framing/OutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/framing/amqp_framing.h 1489458
> /trunk/qpid/cpp/src/qpid/ha/ReplicatingSubscription.cpp 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1489458
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionInputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandler.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/ConnectionOutputHandlerPtr.h 1489458
> /trunk/qpid/cpp/src/qpid/sys/TimeoutHandler.h 1489458
>
> Diff: https://reviews.apache.org/r/11628/diff/
>
>
> Testing
> -------
>
> cmake: make ; make test.
> autotools: make ; make check
>
>
> Thanks,
>
> Andrew Stitcher
>
>