qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Ross <jr...@apache.org>
Subject Re: Review Request 45389: PROTON-1133 - Avoid using hostname as a transport address
Date Thu, 07 Apr 2016 17:57:40 GMT

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


Fix it, then Ship it!





proton-c/bindings/python/proton/reactor.py (line 188)
<https://reviews.apache.org/r/45389/#comment191015>

    I like this name.



proton-c/bindings/python/proton/reactor.py (line 197)
<https://reviews.apache.org/r/45389/#comment191014>

    Perhaps these setters and getters should be python properties.  That seems to be the pattern
elsewhere in proton python, and elsewhere in your patch.



proton-j/src/main/java/org/apache/qpid/proton/reactor/Reactor.java (line 254)
<https://reviews.apache.org/r/45389/#comment191016>

    Recommend putting the annotation on its own line.


- Justin Ross


On April 7, 2016, 2:55 p.m., Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45389/
> -----------------------------------------------------------
> 
> (Updated April 7, 2016, 2:55 p.m.)
> 
> 
> Review request for qpid, Gordon Sim, Justin Ross, and Robbie Gemmell.
> 
> 
> Bugs: PROTON-1133
>     https://issues.apache.org/jira/browse/PROTON-1133
> 
> 
> Repository: qpid-proton-git
> 
> 
> Description
> -------
> 
> This involves a change to the Reactor API.
> 
> This patch implements a new API for creating outgoing connections via the Reactor class:
pn_reactor_connection_to_url().  It is meant to replace the existing practice of creating
a connection via pn_reactor_connection() then setting the transport address in the connection's
hostname field.
> 
> Not 100% convinced this is appropriate.  It introduces a URL parameter to the Proton
Connection object, where it may make better sense to associate the URL with the Transport
instead (pn_reactor_transport_to_url()???).
> 
> The URL parameter is used by the Proton iohandler to create the socket connection.  If
an application does not use the Proton iohandler (by overriding the reactor's global handler),
then it is the responsiblity of whatever handler is being provided to use the URL to set up
the socket connection.  This was also the case for the old method that used the connection's
hostname setting, so this is not a behavioral change.
> 
> 
> Diffs
> -----
> 
>   examples/java/reactor/src/main/java/org/apache/qpid/proton/example/reactor/Send.java
22da720 
>   examples/python/reactor/send.py c718780 
>   examples/python/reactor/tornado-send.py 54b8618 
>   proton-c/bindings/python/proton/__init__.py 5ffede8 
>   proton-c/bindings/python/proton/reactor.py cda6248 
>   proton-c/include/proton/connection.h f8a688c 
>   proton-c/include/proton/reactor.h e91b169 
>   proton-c/src/reactor/connection.c 4a57bfd 
>   proton-c/src/tests/reactor.c 1e706e2 
>   proton-j/src/main/java/org/apache/qpid/proton/engine/Connection.java feff80b 
>   proton-j/src/main/java/org/apache/qpid/proton/reactor/Reactor.java 9d67d49 
>   proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/IOHandler.java 40eddac 
>   proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorImpl.java 0eb126a

>   proton-j/src/test/java/org/apache/qpid/proton/reactor/ReactorTest.java 10c591a 
>   tests/java/org/apache/qpid/proton/ProtonJInterop.java 31306ef 
>   tests/java/shim/creactor.py 95fd020 
> 
> Diff: https://reviews.apache.org/r/45389/diff/
> 
> 
> Testing
> -------
> 
> Updated unit tests and re-checked modified examples.
> 
> 
> Thanks,
> 
> Kenneth Giusti
> 
>


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