Adar Dembo 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. 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/5328/ May want to give Dan a heads-up to make sure he's OK with this alternate approach. 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 making it a standalone class? 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. 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 root of the Kudu source tree (i.e. where the www/ subdirectory can be found). This isn't that, so I don't think we should overload the name. In fact, I'm not really sure what this directory is. Seems like maybe a vendor-specific directory root? 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 as a proxy for where the bin dir should be. 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? -- 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: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Wed, 19 Sep 2018 20:28:07 +0000 Gerrit-HasComments: Yes
