Grant Henke 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 We haven't committed to anything at all yet. I will clearly document our compatibility guarantees when the move to being actually public happens. 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 Done 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. Done 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 See my other answer for more detail. The tl;dr is that a lookup can only occur if the `InetSocketAddress` was created with `new InetSocketAddress(InetAddress addr, int port)`. We can use getHostString() to be sure since the `InetAddress.getByName` call will handle any lookup needed. 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 `InetSocketAddress` will set 1.2.3.4 as the hostname. Methods like `getHostName` and `getHostString` will always return the supplied value. The only time hostname lookups can occur is when the InetSocketAddress is created with `new InetSocketAddress(InetAddress addr, int port)`. 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? This was a carry-over from my WIP "breakout MiniKuduCluster to it's own Module" class. NonRecoverableException is a KuduClient specific exception and I couldn't have the dependency. Outside of that, wrapping in a status object and using a checked exception doesn't really add any value. It results in the same end behavior; a failed test because of an IOException with the error as the message. I can use an IOException to be a bit closer to the NonRecoverableException. 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' Makes sense. I don't think it's possible (currently) to have a server that has never been started before in our BaseKuduTest class. I think thats why I found "restart" confusing. It sounded like it would shutdown and start the server. Perhaps I could enhance this to handle starting a server from scratch. Think that makes sense? 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-21 This isn't KUDU-2186. In fact that issue is fixed by removing HostAndPort from the public APIs. That's what was causing shading issues. This is another case of carryover from my WIP patch. I didn't want the MiniKuduCluster to have a dependency on the kudu-client. -- 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: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Wed, 19 Sep 2018 20:48:12 +0000 Gerrit-HasComments: Yes
