Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic. ...................................................................... Patch Set 2: (2 comments) 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@60 PS1, Line 60: String kuduHomeVar = System.getenv(KUDU_HOME_VAR); > I was just following the ${name}_HOME pattern that is often used. For examp Well, I guess it sort of depends on whether you expect this to be used more from inside a Kudu source tree, or from outside. If the latter, then I agree that KUDU_HOME (a la JAVA_HOME) makes more sense. 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(); > There is no catch or finally. This is a java "try-with-resources Statement Hmm, I'm surprised animalsniffer didn't fail your build; it's configured to disallow any Java API usage above 1.7. On second thought, I guess this isn't an API call but a feature of the language. Could you look into why we don't prohibit that? -- 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: 2 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: Wed, 19 Sep 2018 21:22:48 +0000 Gerrit-HasComments: Yes
