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