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