----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47243/#review132961 -----------------------------------------------------------
Seems good overall, a couple questions/niggles that aren't necessarily worth bothering about. proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/IOHandler.java (line 128) <https://reviews.apache.org/r/47243/#comment197242> This wont do anything unless assertions are explicitly enabled (which often they arent), so it will typically carry on and probably go bang below when trying to use the non-existant host. The C changes protect that explicitly so perhaps the Java changes probably should too. That said, its probably unlikely, and its going to fail either way, so isnt necessarily of much concern. proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorImpl.java (lines 456 - 465) <https://reviews.apache.org/r/47243/#comment197243> We would typically throw an exception (e.g IllegalStateException) in Java rather than silently not do what was asked. Any reason not to here? - Robbie Gemmell On May 11, 2016, 4:52 p.m., Kenneth Giusti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47243/ > ----------------------------------------------------------- > > (Updated May 11, 2016, 4:52 p.m.) > > > Review request for qpid, Alan Conway, Chug Rolke, Cliff Jansen, Justin Ross, > and Robbie Gemmell. > > > Repository: qpid-proton-git > > > Description > ------- > > The pn_connection_set_hostname() interface is used to set the > 'hostname' field in the Open performative. By definition this is the > 'virtual host' and should not be used by reactor for the network > address. The network address for outgoing connections should be set > by using the reactor's pn_reactor_connection_to_host() factory, or the > pn_reactor_set_connection_host() when re-connecting to a different > host. For inbound connections, the peer address is provided by the > acceptor and cannot be modified. In both cases, the > pn_reactor_get_connection_address() method can be used to obtain the > peer's network address. > > > Diffs > ----- > > proton-c/bindings/cpp/src/container_impl.cpp a221f45 > proton-c/bindings/cpp/src/reactor.hpp 48d9ea1 > proton-c/bindings/cpp/src/reactor.cpp 9507d2b > proton-c/bindings/python/proton/reactor.py 1631c35 > proton-c/include/proton/connection.h da20f94 > proton-c/include/proton/reactor.h be642a9 > proton-c/src/posix/io.c 3226594 > proton-c/src/reactor/acceptor.c 8f0e99b > proton-c/src/reactor/connection.c 336d1f1 > proton-c/src/reactor/reactor.h f996dca > proton-c/src/tests/reactor.c 9564569 > proton-c/src/windows/io.c 7ff928d > proton-j/src/main/java/org/apache/qpid/proton/reactor/Reactor.java a3307d2 > > proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/AcceptorImpl.java > fb2f892 > proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/IOHandler.java > 5a32824 > proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorImpl.java > d13cfbe > tests/python/proton_tests/reactor.py 6ee107d > > Diff: https://reviews.apache.org/r/47243/diff/ > > > Testing > ------- > > New unit tests added. > > > Thanks, > > Kenneth Giusti > >
