Dan Burkert 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 definitely makes the changes to finding the binary dir harder to follow. 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 { > Part of why it is separate is that I had considered the case where someone Why would we want an overridable abstraction here, as opposed to requiring the application to set the kuduBinDir property? 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); > Inside the kudu source tree, and most source trees honestly, I expect the s I agree with Adar, we have not used KUDU_HOME in this way previously, and I'd caution against introducing it here. These kind of conventions are really painful to remove once people start using them. As a concrete example, the python and rust test harnesses use KUDU_HOME in a different way: https://github.com/apache/kudu/blob/master/python/setup.py#L104-L124 https://github.com/danburkert/kudu-rs/blob/a059fe9ec4559d8aec80a92a2c3f6601a525caeb/src/mini_cluster.rs#L140-L164 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(); > I guess animal sniffer doesn't check test classes: https://github.com/mojoh try-with-resources is Java 7, and I think this code was more straightforward when it used it. 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'. -- 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 18:51:53 +0000 Gerrit-HasComments: Yes
