> On Dec. 18, 2014, 8:16 p.m., Alan Conway wrote: > > trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp, line 409 > > <https://reviews.apache.org/r/29160/diff/1/?file=794173#file794173line409> > > > > How can this line throw an exception?
Yes, that's a mistake left from an earlier iteration. > On Dec. 18, 2014, 8:16 p.m., Alan Conway wrote: > > trunk/qpid/cpp/src/qpid/messaging/ProtocolRegistry.cpp, line 34 > > <https://reviews.apache.org/r/29160/diff/1/?file=794183#file794183line34> > > > > Factory* is not memory safe, should be a shared_ptr? The registry simply a map of function pointers, not of objects. > On Dec. 18, 2014, 8:16 p.m., Alan Conway wrote: > > trunk/qpid/cpp/src/qpid/messaging/ProtocolRegistry.cpp, line 116 > > <https://reviews.apache.org/r/29160/diff/1/?file=794183#file794183line116> > > > > info is too high for something that happens normally every time you > > connect. Agreed, again just sloppiness on my part. > On Dec. 18, 2014, 8:16 p.m., Alan Conway wrote: > > trunk/qpid/cpp/src/qpid/messaging/Connection.cpp, line 82 > > <https://reviews.apache.org/r/29160/diff/1/?file=794181#file794181line82> > > > > This is strange to me. You iterate over all the factories to create N > > ConnectionImpls in a linked list and then iterate over the impls till one > > works (possibly leaving some unused) > > > > Why not iterate over the factories here and just create impls till one > > works, throwing away those that don't? > > > > I find this eating a linked-list a bit odd. I'm not hugely keen on this either. There were two reasons I ended up doing it for this iteration: (1) the url and map are no longer available on open(). I could get round this by having the impl instance provide accessors to them. At the moment though, e.g the 0-10 impl doesn't store the options as a map. I suppose I could have the superclass hold a version of the original options. (2) There would need to be some extra context passed in to subsequent calls to the registry to request the 'next' implementation. Actually though, on reflection, I think I'm possibly over complicating things and could just request am impl by name and hard code an order into open, avoiding the need for any extra state. I'll try that, it should clean things up a little. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29160/#review65373 ----------------------------------------------------------- On Dec. 17, 2014, 6:20 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29160/ > ----------------------------------------------------------- > > (Updated Dec. 17, 2014, 6:20 p.m.) > > > Review request for qpid, Alan Conway, Chug Rolke, and Justin Ross. > > > Bugs: QPID-6256 > https://issues.apache.org/jira/browse/QPID-6256 > > > Repository: qpid > > > Description > ------- > > * have a specific exception for signalling protocol mismatch up the stack > * use this to try alternative(s) in the event protocol is not explicitly > specified and first choice isn't supported by peer > > > Diffs > ----- > > trunk/qpid/cpp/include/qpid/messaging/exceptions.h 1646261 > trunk/qpid/cpp/src/qpid/Exception.h 1646261 > trunk/qpid/cpp/src/qpid/client/Connection.cpp 1646261 > trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp 1646261 > trunk/qpid/cpp/src/qpid/client/Connector.h 1646261 > trunk/qpid/cpp/src/qpid/client/Connector.cpp 1646261 > trunk/qpid/cpp/src/qpid/client/RdmaConnector.cpp 1646261 > trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1646261 > trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1646261 > trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1646261 > trunk/qpid/cpp/src/qpid/framing/ProtocolInitiation.h 1646261 > trunk/qpid/cpp/src/qpid/messaging/Connection.cpp 1646261 > trunk/qpid/cpp/src/qpid/messaging/ConnectionImpl.h 1646261 > trunk/qpid/cpp/src/qpid/messaging/ProtocolRegistry.cpp 1646261 > trunk/qpid/cpp/src/qpid/messaging/exceptions.cpp 1646261 > > Diff: https://reviews.apache.org/r/29160/diff/ > > > Testing > ------- > > * make test passes > * connects to qpidd when 0-10 has been disabled on broker > * connects to rabbitmq > * didn't connect to activemq 5.10 due to AMQ-5490, but that is fixed so > should work against trunk (to be verified) > > > Thanks, > > Gordon Sim > >
