qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Conway" <acon...@redhat.com>
Subject Re: Review Request 29160: Allow automatically upgrading to 1.0 if 0-10 is neither required nor supported by peer
Date Thu, 18 Dec 2014 20:16:50 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29160/#review65373
-----------------------------------------------------------


Looks like it would work, but I found it a little odd in places.


trunk/qpid/cpp/src/qpid/client/ConnectionImpl.cpp
<https://reviews.apache.org/r/29160/#comment108475>

    How can this line throw an exception?



trunk/qpid/cpp/src/qpid/messaging/Connection.cpp
<https://reviews.apache.org/r/29160/#comment108481>

    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.



trunk/qpid/cpp/src/qpid/messaging/ProtocolRegistry.cpp
<https://reviews.apache.org/r/29160/#comment108483>

    Factory* is not memory safe, should be a shared_ptr?



trunk/qpid/cpp/src/qpid/messaging/ProtocolRegistry.cpp
<https://reviews.apache.org/r/29160/#comment108477>

    info is too high for something that happens normally every time you connect.



trunk/qpid/cpp/src/qpid/messaging/ProtocolRegistry.cpp
<https://reviews.apache.org/r/29160/#comment108478>

    result should be an intrusive_ptr for consistency of memory management.
    
    Why a C list here? Wouldn't a vector be less error-prone?


- Alan Conway


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


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