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