Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12351 )
Change subject: KUDU-2411: Set SASL_PATH if needed when starting MiniCluster ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/12351/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryArtifactInfo.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryArtifactInfo.java: http://gerrit.cloudera.org:8080/#/c/12351/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryArtifactInfo.java@27 PS4, Line 27: public class KuduBinaryArtifactInfo { I think a constructor and getters is best here to prevent bad mutation practices. Nit: I think KuduBinaryInfo is a detailed enough name http://gerrit.cloudera.org:8080/#/c/12351/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java: http://gerrit.cloudera.org:8080/#/c/12351/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java@143 PS4, Line 143: return props; We could construct and retun the full object here. http://gerrit.cloudera.org:8080/#/c/12351/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryLocator.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryLocator.java: http://gerrit.cloudera.org:8080/#/c/12351/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryLocator.java@84 PS4, Line 84: return extractor.extractKuduBinaryArtifact(testTmpDir); We can set the binDir in the constructor inside this method since the extract knows where the binary will be placed. http://gerrit.cloudera.org:8080/#/c/12351/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryLocator.java@100 PS4, Line 100: info.binDir = kuduBinDir; Can we just return a new info object here with the sasl path unset. http://gerrit.cloudera.org:8080/#/c/12351/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryLocator.java@127 PS4, Line 127: return exeInfo; We could construct this object at the end instead of mutating. nit: I like the return at the bottom. Perhaps use the if statement on the throw clause. -- To view, visit http://gerrit.cloudera.org:8080/12351 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaaeb30781f4483910c35a20c6d7c76f7f85aa4ce Gerrit-Change-Number: 12351 Gerrit-PatchSet: 4 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Brian McDevitt <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 06 Feb 2019 04:26:50 +0000 Gerrit-HasComments: Yes
