Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14398 )
Change subject: IMPALA-8974: Fixed a bug when create kudu managerd table without HMS config ...................................................................... Patch Set 5: (5 comments) Thank you for adding the test. I have some concerns about how it changes config files around, so I proposed an alternative solution. http://gerrit.cloudera.org:8080/#/c/14398/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14398/5//COMMIT_MSG@7 PS5, Line 7: managerd spelling: managed (also a few cases below). http://gerrit.cloudera.org:8080/#/c/14398/5/fe/src/test/java/org/apache/impala/customcluster/CreateKuduTableWithoutHMSTest.java File fe/src/test/java/org/apache/impala/customcluster/CreateKuduTableWithoutHMSTest.java: http://gerrit.cloudera.org:8080/#/c/14398/5/fe/src/test/java/org/apache/impala/customcluster/CreateKuduTableWithoutHMSTest.java@35 PS5, Line 35: managerd spelling: managed http://gerrit.cloudera.org:8080/#/c/14398/5/fe/src/test/java/org/apache/impala/customcluster/CreateKuduTableWithoutHMSTest.java@38 PS5, Line 38: public class CreateKuduTableWithoutHMSTest { Usually we write this kind of custom cluster test in Python instead of Java, except for special cases where the test requires a java library (like LDAP). I think this is OK and I don't want to ask you to rewrite the test, but just in case you write one in future, I would use one of the tests under tests/custom_cluster/ as an example. http://gerrit.cloudera.org:8080/#/c/14398/5/fe/src/test/java/org/apache/impala/customcluster/CreateKuduTableWithoutHMSTest.java@66 PS5, Line 66: managerd spelling: managed http://gerrit.cloudera.org:8080/#/c/14398/5/fe/src/test/java/org/apache/impala/customcluster/CreateKuduTableWithoutHMSTest.java@84 PS5, Line 84: String replace_cmd = String.format("mv %s %s", HIVE_SITE_WITHOUT_HMS, HIVE_SITE); I don't really like this test modifying global test configs, it really risks messing up someone's dev environment and having a hard time debugging things. I think it would be better to put the generated hive-site.xml in a different directory, e.g. /tmp/CreateKuduTableWithoutHMSTest and then add that to the classpath for the test. You would need to prepend it to the CLASSPATH environment variable in start-daemon.sh. Maybe you could have something like this in start-daemon.sh: . ${IMPALA_HOME}/bin/set-classpath.sh if [[ -v TEST_PREPEND_CLASSPATH ]]; then CLASSPATH=$TEST_PREPEND_CLASSPATH:$CLASSPATH fi Then your test could set TEST_PREPEND_CLASSPATH, or pass it as a flag to start-impala-cluster.py -- To view, visit http://gerrit.cloudera.org:8080/14398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacc53801a660c033869cb4747910c98a80e08297 Gerrit-Change-Number: 14398 Gerrit-PatchSet: 5 Gerrit-Owner: wangsheng <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Mon, 21 Oct 2019 21:43:12 +0000 Gerrit-HasComments: Yes
