----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19508/#review38412 -----------------------------------------------------------
Thanks Michi for the patch, it looks very nice. I've just few minor suggestions, kindly see it. http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java <https://reviews.apache.org/r/19508/#comment70565> Since we are touching this, can we append the strings instead of concatenation ? like: sw.append(HostNameUtils.getHostString(clientAddr)); sw.append(":"); sw.append(String.valueOf(clientAddr.getPort())); http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/test/org/apache/zookeeper/common/HostNameUtilsTest.java <https://reviews.apache.org/r/19508/#comment70561> Very good test cases. It would be nice if we add the message section in the Assert.assertEquals("message", expAg, actualArg) like: Assert.assertEquals("Failed to resolve InetSocketAddress with no host to 0.0.0.0", socketAddress.getAddress().getHostAddress(), HostNameUtils.getHostString(socketAddress); Also it would be great if we can modify other assertions present in the tests too. - Rakesh R On March 20, 2014, 11:52 p.m., michim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19508/ > ----------------------------------------------------------- > > (Updated March 20, 2014, 11:52 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper > > > Description > ------- > > Add a method to get the hostname or the ip address of InetSocketAddress > without performing a reverse DNS lookup. It's basically a substitute for > java.net.InetSocketAddress#getHostString(), which is only available since > java 7. > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/common/HostNameUtils.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java > 1579834 > > http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/test/org/apache/zookeeper/common/HostNameUtilsTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/19508/diff/ > > > Testing > ------- > > Added unit tests, and verified the patch fixes ObserverTest.testObserver. > > > Thanks, > > michim > >
