qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Robbie Gemmell <robbie.gemm...@gmail.com>
Subject Re: Review Request 45389: WIP PROTON-1133 - Avoid using hostname as a transport address
Date Mon, 04 Apr 2016 10:32:51 GMT

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



Updated review of rev3.

There are two 'setConnectionAddress' methods in the Java changes, but only one in the equivalent
C changes. I dont think we need the simple no-port one, it doesnt seem unreasonable to force
people to specify a port they want to connect to in this sort of code (the Java bits could
do exactly what the C does with a null value...though, see also next point).

Using a String rather than an int for the port feels a bit odd, with other bits of the Java
changes even having to convert an existing int to pass the value. That said, I can see it
possibly aligns slightly better with the single-String 'get address' return value.

I would move the new 'url' arg handling from the Send constructor (in Send.java) to the main
method alongside the other arg parsing, then passing the host and port values to the Send
constructor. Keeps all the arg handling in once place at the start where its typically done.

There are various places in the new Java changes (such as the above bit) that use if/else
statements without braces, please add the braces everywhere :)

With my earlier comments I was actually thinking that if the connection 'attachments' were
used to store the details we could just define the key(s) used to store the host/port/combined-value,
then the reactor.connection(handler, host, port) style method and IO handler (plus anything
else wanting to update/inspect them, such as the python Connector) would use the connection
attachments directly to set/get the value(s) rather than defining any new methods to do it.
Though it may be more obvious to set/get the details on the connection object itself rather
than via the reactor object, the methods are definitely nicer in some ways since using the
attachments is rather verbose and folks wont need to know the keys this way (though could
of course still do it that way given its all the methods do ultiamtely). I think I would have
retained the reactor.connection(handler, host, port) style method even if adding 'set adddress'
style method though, I think it was more intuitive.

- Robbie Gemmell


On April 3, 2016, 11:34 p.m., Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45389/
> -----------------------------------------------------------
> 
> (Updated April 3, 2016, 11:34 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/reactor.py cda6248 
>   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/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