Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11475 )
Change subject: [test] Adjust Kudu binary locator logic. ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/11475/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11475/1//COMMIT_MSG@9 PS1, Line 9: This patch adjusts the binary locating logic to be less fragile and more externally useful. It also breaks the > Nit: line too long. Done http://gerrit.cloudera.org:8080/#/c/11475/1//COMMIT_MSG@17 PS1, Line 17: This also fixes : the issue where Maven tests would fail to locate the : binary because they were using jars installed into : the local repository. > I presume you've already read through this: https://gerrit.cloudera.org/c/5 Added him to the review and pinged out of band. Based on that discussion, I don't think he will have an issue. 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 { > This is only used by the MiniKuduCluster. Perhaps move it there instead of Part of why it is separate is that I had considered the case where someone might want to override the logic here. I could add a setter to the MiniKuduCluster allowing you to provide your own instance. I didn't do that yet, but I could in this patch. http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@53 PS1, Line 53: LOG.info("Using Kudu binary directory specified by system property '{}': {}", KUDU_BIN_DIR_PROP, kuduHomeProp); > Some of these lines look to be too long. Done http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@60 PS1, Line 60: LOG.info("Using Kudu home directory specified by environment variable '{}': {}", KUDU_HOME_VAR, kuduHomeVar); > In various places in the codebase, the KUDU_HOME env variable refers to the I was just following the ${name}_HOME pattern that is often used. For example, JAVA_HOME. This way KUDU_HOME could be exported to indicate the home directory of the Kudu install. Any ideas for a better environment variable to use? The ${name}_HOME pattern seams extremely common. http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@65 PS1, Line 65: // Last, use the kudu that is available on the path. > That's not quite what this is doing though; it's using 'kudu' on the path a Done http://gerrit.cloudera.org:8080/#/c/11475/1/java/kudu-client/src/test/java/org/apache/kudu/util/KuduBinaryLocator.java@71 PS1, Line 71: try (final Reader reader = new InputStreamReader(process.getInputStream(), UTF_8)) { > Where's the 'catch' or finally' for this 'try'? Is it L78? There is no catch or finally. This is a java "try-with-resources Statement" that will automatically handle closing the reader on error or success. It's effectively an automatic finally clause. This is a Java 8 feature though, so perhaps I should remove this. -- 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: 1 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:18:03 +0000 Gerrit-HasComments: Yes
