Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21153 )
Change subject: IMPALA-12913: Refactor Workload Management Custom Cluser Tests ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/21153/1/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/21153/1/testdata/datasets/functional/functional_schema_template.sql@1804 PS1, Line 1804: CREATE EXTERNAL TABLE IF NOT EXISTS {db_name}{db_suffix}.{table_name} ( > There seem to be support for all this above with COLUMNS, ROW_FORMAT, DEPEN Agree, using COLUMNS section might be better. http://gerrit.cloudera.org:8080/#/c/21153/1/tests/custom_cluster/test_query_log.py File tests/custom_cluster/test_query_log.py: http://gerrit.cloudera.org:8080/#/c/21153/1/tests/custom_cluster/test_query_log.py@44 PS1, Line 44: super(TestQueryLogTableBase, cls).add_test_dimensions() Add the following after L44 cls.ImpalaTestMatrix.add_dimension(create_single_exec_option_dimension()) This will only test with disable_codegen_options=False. http://gerrit.cloudera.org:8080/#/c/21153/1/tests/custom_cluster/test_query_log.py@438 PS1, Line 438: client = self.create_impala_client(protocol=vector.get_value('protocol')) > Why do you create a client? ImpalaTestSuite and its subclasses initialize 3 clients for each test: client (beeswax), hs2_client (HS2), and hs2_http_client (HS2 over HTTP) https://github.com/apache/impala/blob/4477398ae46415d3fb32db2a8fd5e6d2060cbd3f/tests/common/impala_test_suite.py#L322-L353 run_test_case() use them, and they are closed automatically after test run complete. So no need to declare try-finally. But the benefit stops there. Reusing them is probably good if test only need one client of that kind. But once the test gets complex (using 2 client like this method, or using create_client_for_nth_impalad), I think it is fine to doo it this way too. http://gerrit.cloudera.org:8080/#/c/21153/1/tests/custom_cluster/test_query_log.py@614 PS1, Line 614: @pytest.mark.parametrize("buffer_pool_limit", [None, "14.97MB"]) : def test_query_log_table_query_select(self, vector, buffer_pool_limit): Contain this test in its own test class, and define buffer_pool_limit test dimension just for this. For example: class TestQueryLogTableBufferPool(CustomClusterTestSuite): """Tests to assert the query log table is correctly populated.""" @classmethod def add_test_dimensions(cls): super(TestQueryLogTableBufferPool, cls).add_test_dimensions() add_exec_option_dimension(cls, 'buffer_pool_limit', [None, '16.05MB']) add_mandatory_exec_option(cls, 'max_mem_estimate_for_admission', '10MB') @CustomClusterTestSuite.with_args(impalad_args="--enable_workload_mgmt " "--query_log_write_interval_s=1 " "--cluster_id=test_query_hist_1 " "--shutdown_grace_period_s=10 " "--shutdown_deadline_s=60 " "--scratch_dirs={0}:5G" .format(SCRATCH_DIR), catalogd_args="--enable_workload_mgmt", impalad_graceful_shutdown=True) def test_query_log_table_query_select(self, vector, unique_database): """Asserts the values written to the query log table match the values from the query profile. If the buffer_pool_limit parameter is not None, then this test requires that the query spills to disk to assert that the spill metrics are correct in the completed queries table.""" tbl_name = unique_database + ".test_query_log" client_protocol = vector.get_value('protocol') buffer_pool_limit = vector.get_value('exec_option')['buffer_pool_limit'] client = self.create_impala_client(protocol=client_protocol) http://gerrit.cloudera.org:8080/#/c/21153/1/tests/custom_cluster/test_query_log.py@635 PS1, Line 635: client.set_configuration_option("SPOOL_QUERY_RESULTS", "TRUE") Can probably remove this, since it is True by default. -- To view, visit http://gerrit.cloudera.org:8080/21153 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e3249a8f306cf43de0d6f6586711c779399e83b Gerrit-Change-Number: 21153 Gerrit-PatchSet: 1 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Mon, 18 Mar 2024 22:28:06 +0000 Gerrit-HasComments: Yes
