Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14962 )
Change subject: IMPALA-9266: Use custom cluster test case to replace CreateKuduTableWithoutHMSTest.java ...................................................................... Patch Set 5: (7 comments) Thanks for uploading the patch so quickly! Added some comments. http://gerrit.cloudera.org:8080/#/c/14962/5/bin/create-test-configuration.sh File bin/create-test-configuration.sh: http://gerrit.cloudera.org:8080/#/c/14962/5/bin/create-test-configuration.sh@143 PS5, Line 143: if [ "$WITHOUT_HMS" = "true" ]; then : export KUDU_VARIANT=without_hms_config : fi The config should be generated each time we run bin/create-test-configuration.sh. Just like other config files. So these should be something like: export KUDU_VARIANT=without_hms_config $IMPALA_HOME/bin/generate_xml_config.py hive-site.xml.py hive-site_without_hms.xml BTW, KUDU_VARIANT is not a quite relative name since this is actually another configuration for Hive, not for Kudu. We can use HIVE_VARIANT directly. http://gerrit.cloudera.org:8080/#/c/14962/5/tests/custom_cluster/test_kudu_table_create_without_hms.py File tests/custom_cluster/test_kudu_table_create_without_hms.py: http://gerrit.cloudera.org:8080/#/c/14962/5/tests/custom_cluster/test_kudu_table_create_without_hms.py@25 PS5, Line 25: HIVE_SITE_EXT_DIR = IMPALA_HOME + '/fe/src/test/resources/hive-site-ext' Please use another dir. 'hive-site-ext' is used for hive-site.xml with non-default "hive.metastore.warehouse.external.dir". http://gerrit.cloudera.org:8080/#/c/14962/5/tests/custom_cluster/test_kudu_table_create_without_hms.py@29 PS5, Line 29: TestKuduTableCreateWithoutHMS nit: TestCreatingKuduTableWithoutHMS is more correct in grammar. http://gerrit.cloudera.org:8080/#/c/14962/5/tests/custom_cluster/test_kudu_table_create_without_hms.py@35 PS5, Line 35: os.system("bash %s/bin/create-test-configuration.sh" % IMPALA_HOME) We should not run bin/create-test-configuration.sh everytime we run this test. It has side-effect that may restore other config files. So we don't need this setup func. http://gerrit.cloudera.org:8080/#/c/14962/5/tests/custom_cluster/test_kudu_table_create_without_hms.py@42 PS5, Line 42: class ThreadLocalClient(threading.local): : def __init__(self): : self.client = test_self.create_impala_client() : : tls = ThreadLocalClient() Don't need to create the client like this. This is a single thread test. You can use self.execute_query_expect_success directly. There're many examples in other tests. http://gerrit.cloudera.org:8080/#/c/14962/5/tests/custom_cluster/test_kudu_table_create_without_hms.py@48 PS5, Line 48: create table %s " : "(id int, primary key(id)) stored as kudu We should create the table in a unique database. Just declare the function with another one argument 'unique_database'. Then a unique database with this name will be created before running this test, and dropped after finishing this test. Some details if you're interested: https://github.com/apache/impala/blob/b02f51458396e5d9fcb1444ae9d33642cbf9580f/tests/conftest.py#L238 You can find many examples using the unique_database in other tests. http://gerrit.cloudera.org:8080/#/c/14962/5/tests/custom_cluster/test_kudu_table_create_without_hms.py@56 PS5, Line 56: os.system("bash %s/bin/create-test-configuration.sh" % IMPALA_HOME) Same here. Don't need teardown. -- To view, visit http://gerrit.cloudera.org:8080/14962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic532574af42ed864612cf28eecee9e0416ef272c Gerrit-Change-Number: 14962 Gerrit-PatchSet: 5 Gerrit-Owner: wangsheng <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Mon, 30 Dec 2019 13:27:38 +0000 Gerrit-HasComments: Yes
