qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gordon Sim" <g...@redhat.com>
Subject Re: Review Request: C++ code spring cleaning - mostly removing unused stuff, and tidying up some of the interfaces
Date Thu, 06 Jun 2013 15:23:08 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message