----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45389/#review126337 -----------------------------------------------------------
Looks good other than the couple of tiny issues noted. proton-j/src/main/java/org/apache/qpid/proton/engine/Connection.java (lines 126 - 127) <https://reviews.apache.org/r/45389/#comment189310> Does this need to be added to the interface? The only use of it seems to cast to impl anyway, and it is asymmetric given there isn't an equivalent setter being added. I'd make it impl-only for now at least if possible to avoid more reactor-specific leakage into the general API (ugly, but so is the lack of a 'ReactorConnection' equivalent to match the C stuff). proton-j/src/main/java/org/apache/qpid/proton/reactor/Reactor.java (line 265) <https://reviews.apache.org/r/45389/#comment189313> The url param goes before the handler in the C variant, which might be more natural, but I think consistency either way would be good. - Robbie Gemmell On March 29, 2016, 8:27 p.m., Kenneth Giusti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45389/ > ----------------------------------------------------------- > > (Updated March 29, 2016, 8:27 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/engine/Connection.java > feff80b > > proton-j/src/main/java/org/apache/qpid/proton/engine/impl/ConnectionImpl.java > b708d83 > 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 > > Diff: https://reviews.apache.org/r/45389/diff/ > > > Testing > ------- > > Updated unit tests and re-checked modified examples. > > > Thanks, > > Kenneth Giusti > >
