Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11474 )
Change subject: [test] Clean up MiniKuduCluster and BaseKuduTest ...................................................................... Patch Set 2: (3 comments) 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: getHostString > See my other answer for more detail. The tl;dr is that a lookup can only oc More broadly, this is the main issue I have with InetSocketAddress: it's easy to accidentally introduce DNS lookups. Yes, you've ensured that the current usage is lookup free, but what's stopping someone from seeing that an API expects an InetSocketAddress and building one with the wrong constructor, adding a forward lookup? Or creating an InetSocketAddress around an InetAddress, then calling getHostName() call by mistake, adding a reverse lookup? This is what made HostAndPort nice: it guarantees never doing any lookups. If you think switching away from HostAndPort is a big improvement, is there any way we can retain that guarantee? Maybe: 1. Extend InetSocketAddress and bake in our own enforcement, including throwing on getHostName(), and disallowing certain constructors. 2. Roll our own basic HostAndPort. 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 IOException(resp.getError().getMessage()); > This was a carry-over from my WIP "breakout MiniKuduCluster to it's own Mod I guess I don't understand the dependency issue; are you saying MiniKuduCluster can't depend on anything from kudu-client? What about the various protobufs that it uses? Or SecurityUtil? I don't actually care about the exception. 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 { > Looking again, the MiniKuduCluster starts all the servers when `build()` is I don't think you'll be able to get starting a server from scratch working; the C++ mini cluster doesn't support that. Anyway, I don't care strongly about the naming; just wanted to make sure you were aware of the underlying limitation. -- 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: 2 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Comment-Date: Wed, 19 Sep 2018 21:12:47 +0000 Gerrit-HasComments: Yes
