qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kenneth Giusti <kgiu...@apache.org>
Subject Re: Review Request 45389: WIP PROTON-1133 - Avoid using hostname as a transport address
Date Mon, 04 Apr 2016 20:31:43 GMT


> On April 4, 2016, 10:32 a.m., Robbie Gemmell wrote:
> > 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.

Hi Robbie,

Thanks for the feedback, couple of points:

First, I'd much rather use integer port types, but on the C side the acceptor constructor
uses a string-based port, so I kept the interface consistent (consistently wrong one might
say, but that's due to the underlying call to getaddrinfo()).   I'll modify the Java bits
to use an integer to be consistent with its Acceptor implementation.

Yes, I'll fix the braces - one can never be too safe ;)

Hrmmm - personally I'd rather stay away from using the attachments directly in this case.
 Setting the host/port when using the reactor connection is mandatory, so that deserves a
simple API.  Handling failover is likely to also see a lot of usage - we should make resetting
the host/port as easy as possible. And the C api for attachments is a bit verbose.

Lastly - I'm going to add back that connection constructor that takes the host and port, and
make the changes to the Send constructor you suggest.


- Kenneth


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


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