Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/11474/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11474/1//COMMIT_MSG@11 PS1, Line 11: public test API We're not committing to this with the same level of rigor or compatibility that we would our normal client API, right? What sort of guarantees do you have in mind? Could you spell them out? http://gerrit.cloudera.org:8080/#/c/11474/1//COMMIT_MSG@15 PS1, Line 15: part of the clean up. It replaces it with unresolved InetSocketAddress because HostAndPort is provided Nit: got a too long line here http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java: http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@345 PS1, Line 345: public static InetSocketAddress AddressFromPB(Common.HostPortPB hostPortPB) { Should be addressFromPB. http://gerrit.cloudera.org:8080/#/c/11474/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: http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@77 PS1, Line 77: getHostName() Should we use getHostString() in place of getHostName()? The documentation says getHostName() may do a reverse lookup. Whether that happens or doesn't depends on how the InetSocketAddress was built (the lookup happens "if the address was created with a literal IP address"), but I don't know if that condition means the InetSocketAddress must have been constructed with a InetAddress argument, or with a hostname of "1.2.3.4" (see my other question about this in NetUtil). http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java File java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java: http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@67 PS1, Line 67: return InetSocketAddress.createUnresolved(hostAndPort.getHost(), hostAndPort.getPort()); How does the resulting InetSocketAddress behave when the 'host' argument is actually an IP address string literal, like "1.2.3.4"? Does it still consider that string to be a hostname and the object is "unresolved"? http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java: http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@178 PS1, Line 178: throw new RuntimeException(resp.getError().getMessage()); Why this change? http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@297 PS1, Line 297: public void startMasterServer(InetSocketAddress address) throws IOException { FWIW, the reason these methods were prefixed with 'start' and not 'restart' is because you can't use them to start a server from scratch; you can only bring back up a server that was previously started when the cluster itself was started, and subsequently killed. http://gerrit.cloudera.org:8080/#/c/11474/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java@479 PS1, Line 479: /** : * Converts HostPortPB to an unresolved InetSocketAddress. : * : * @param pb the host and port to convert : * @return an unresolved InetSocketAddress : */ : private static InetSocketAddress addressFromPB(HostPortPB pb) { : return InetSocketAddress.createUnresolved(pb.getHost(), pb.getPort()); : } Can't reuse the version in ProtobufHelper? Is this more of the TODO(KUDU-2186) thing from below? -- To view, visit http://gerrit.cloudera.org:8080/11474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a55c41f48f31c4a12b41508dbb343c4419e65b1 Gerrit-Change-Number: 11474 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Wed, 19 Sep 2018 20:17:01 +0000 Gerrit-HasComments: Yes
