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: (2 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: getHostName() > More broadly, this is the main issue I have with InetSocketAddress: it's ea I think both of those are options. I don't want to roll all the parsing and validation logic. I suppose we could make our own HostAndPort that holds and instance of InetSocketAddress internally. The main reason we can't use HostAndPort is because we shade and relocate Guava. This means that it should not be a parameter or return value for any "public" methods. Especially if they are to be used across modules. 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()); > I guess I don't understand the dependency issue; are you saying MiniKuduClu Yeah, that is what I am saying. It will take some more changes to make that work. Like you pointed out SecurityUtils and Protobufs. I am talking about follow on work outside of this patch where MiniKuduCluster is in it's own module. If that module needed kudu-client then we couldn't use it in the kudu-client tests. It's a dependency ordering problem. I think Gradle could handle it, but not Maven. Even still it's worth keeping them separate. I think it would be easier to explain outside of chat, or show with a sample patch. I am working on those patches. -- 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 21:27:46 +0000 Gerrit-HasComments: Yes
