You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Kenneth Giusti <kg...@apache.org> on 2016/04/07 15:30:55 UTC
Re: Review Request 45389: PROTON-1133 - Avoid using hostname as a
transport address
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45389/
-----------------------------------------------------------
(Updated April 7, 2016, 1:30 p.m.)
Review request for qpid, Gordon Sim, Justin Ross, and Robbie Gemmell.
Changes
-------
Last change - promise!
I don't like the term "address" used by the setConnectionAddress/set_connection_address methods. It really is too generic. To be specific, these APIs allow you to set the *host address*. This patch simply renames the set method to use Host rather than Address.
This allows for the pontential addition of a proper URL based api should be decide to got that route.
I'm ok with the getConnectionAddress/get_connection_address naming, since this returns a string representation that is compatible with proton's Url class.
Summary (updated)
-----------------
PROTON-1133 - Avoid using hostname as a transport address
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 (updated)
-----
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
Re: Review Request 45389: PROTON-1133 - Avoid using hostname as a
transport address
Posted by Robbie Gemmell <ro...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45389/#review127597
-----------------------------------------------------------
proton-j/src/main/java/org/apache/qpid/proton/reactor/Reactor.java (lines 244 - 257)
<https://reviews.apache.org/r/45389/#comment190987>
I would just shorten the @deprecated description to "Use {@link #connectionToHost(String, int, Handler)} instead" rather than go on to describe the old way of doing things (espcially as it wasn't ever described previously hehe, and conflicts with the new description for setConnectionHost just below). The @deprecated should probably go last too.
Other than that, looks good to me.
- Robbie Gemmell
On April 7, 2016, 1:30 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, 1:30 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
>
>
Re: Review Request 45389: PROTON-1133 - Avoid using hostname as a
transport address
Posted by Kenneth Giusti <kg...@apache.org>.
> On April 7, 2016, 5:57 p.m., Justin Ross wrote:
> > proton-c/bindings/python/proton/reactor.py, line 197
> > <https://reviews.apache.org/r/45389/diff/6/?file=1330014#file1330014line197>
> >
> > Perhaps these setters and getters should be python properties. That seems to be the pattern elsewhere in proton python, and elsewhere in your patch.
I'm not sure they can. The host and port are actually associated with the _connection_, not the _reactor_. I crafted these APIs so they belong to the reactor API because we didn't want host/port information leaking into the AMQP Connection API. We can't make them a property of the Reactor, since we have to specify the connection. And I really don't want to associate host and port properties with the connection class.
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45389/#review127631
-----------------------------------------------------------
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
>
>
Re: Review Request 45389: PROTON-1133 - Avoid using hostname as a
transport address
Posted by Justin Ross <jr...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 45389: PROTON-1133 - Avoid using hostname as a
transport address
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
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 (updated)
-----
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