Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic. ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/11475/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11475/3//COMMIT_MSG@10 PS3, Line 10: It also breaks the : logic out of the TestUtils class into it’s own class. : I broke out the remaining TestUtils methods as : well > I'm -0 on the proliferation of util classes. What's the motivation? It de Part of the reason is I plan to move some of these classes around in the future to enable breaking out the MiniKuduCluster functionality for general integration testing outside the project. http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java File java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java: http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@35 PS1, Line 35: public class KuduBinaryLocator { > Why would we want an overridable abstraction here, as opposed to requiring This is preliminary work to breaking out a separate module/jar for the KuduMiniCluster to support simplified integration testing. I expect that would be a workaround for any case that doesn't fit existing options. http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@60 PS1, Line 60: String kuduHomeVar = System.getenv(KUDU_HOME_VAR); > I agree with Adar, we have not used KUDU_HOME in this way previously, and I I will remove the KUDU_HOME option. The system property and path should be flexible enough. http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@71 PS1, Line 71: Runtime runtime = Runtime.getRuntime(); > try-with-resources is Java 7, and I think this code was more straightforwar Done http://gerrit.cloudera.org:8080/#/c/11475/3/java/kudu-client/src/test/java/org/apache/kudu/util/ProtocolUtils.java File java/kudu-client/src/test/java/org/apache/kudu/util/ProtocolUtils.java: http://gerrit.cloudera.org:8080/#/c/11475/3/java/kudu-client/src/test/java/org/apache/kudu/util/ProtocolUtils.java@29 PS3, Line 29: public class ProtocolUtils { > I think this would be better named 'ProtobufUtils'. Done -- To view, visit http://gerrit.cloudera.org:8080/11475 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I365dfb9fdeeb4fd985f370a6b8305345e0a2ac7d Gerrit-Change-Number: 11475 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Thu, 20 Sep 2018 19:19:26 +0000 Gerrit-HasComments: Yes
