Todd Lipcon has posted comments on this change. Change subject: [java] KUDU-2103 Canonicalize hostnames in client ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/7757/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java: Line 74: public String getHostname() { I think it's bad form to hide a potentially-expensive DNS request in what looks like a simple getter here. Instead can we put this in a new method in Connection.java where serverInfo.getHostname() is used, so it's more explicit? http://gerrit.cloudera.org:8080/#/c/7757/1/java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java File java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java: Line 64: Method method = InetAddress.class.getDeclaredMethod("getCanonicalHostName"); unused? PS1, Line 80: if (host.equals("master1.example.com") || host.equals("server123.example.com")) { : return new InetAddress[]{InetAddress.getByAddress(new byte[]{10, 1, 2, 3})}; : } I think it's a bit messy to have this forward/reverse mapping hard-coded in the implementation here rather than in the one test that requires it. How about adding some Map<String, InetAddress[]> and Map<InetAddress, String> members to FakeDNS and have the test that needs this mapping insert the desired resolutions? http://gerrit.cloudera.org:8080/#/c/7757/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java: Line 41: public void testGetUnknownHostname() throws Exception { can you add some comments here explaining what these test cases are asserting? Line 47: InetAddress ia = InetAddress.getByName("8.8.8.8"); I don't understand the comment above. Can you make it a full sentence and describe what behavior this is working around, etc? -- To view, visit http://gerrit.cloudera.org:8080/7757 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
