Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22617 )
Change subject: IMPALA-13861: Standardize workload management tests ...................................................................... Patch Set 6: (18 comments) http://gerrit.cloudera.org:8080/#/c/22617/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22617/5//COMMIT_MSG@9 PS5, Line 9: worklo > I think "workload management" instead of "system" would be clearer here. T Done http://gerrit.cloudera.org:8080/#/c/22617/5//COMMIT_MSG@18 PS5, Line 18: test_workload_mgmt_sql_details.py are refactored to extend from > Please also mention that test_workload_mgmt_sql_details.py now uses 1 minic Done http://gerrit.cloudera.org:8080/#/c/22617/5//COMMIT_MSG@40 PS5, Line 40: differ > Nit: enables Done http://gerrit.cloudera.org:8080/#/c/22617/5//COMMIT_MSG@43 PS5, Line 43: > Nit: changes Done http://gerrit.cloudera.org:8080/#/c/22617/5/tests/common/wm_test_suite.py File tests/common/wm_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22617/5/tests/common/wm_test_suite.py@25 PS5, Line 25: wait > nit: waits Done http://gerrit.cloudera.org:8080/#/c/22617/5/tests/common/wm_test_suite.py@26 PS5, Line 26: wait > nit: waits Done http://gerrit.cloudera.org:8080/#/c/22617/5/tests/common/wm_test_suite.py@27 PS5, Line 27: reach > nit: reaches Done http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_admission_controller.py@1902 PS5, Line 1902: workload_mgmt=True, impalad_args=impalad_admission_ctrl_config_args( > Since this change automatically sets --query_log_write_interval_s to 1 when I don't think it matters, because workload_mgmt enables graceful shutdown by default now, and it will flush query log. http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_live.py File tests/custom_cluster/test_query_live.py: http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_live.py@79 PS5, Line 79: @CustomClusterTestSuite.with_args(impalad_args="--cluster_id=test_query_live", > Since this change automatically sets --query_log_write_interval_s to 1 when Correct. CustomClusterTestSuite._start_impala_cluster, however, is an exception because it is oblivious about workload_mgmt argument. http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_live.py@189 PS5, Line 189: # full --query_log_write_interval_s to pass. > I'm not sure why this extra restart is needed since the `drop table` dml is Couple restart added here mainly is to bypass high --query_log_write_interval_s=300. Remember that WorkloadManagementTestSuite will wait until impala-server.completed-queries.queued metric reach 0. http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_live.py@303 PS5, Line 303: # full --query_log_write_interval_s to pass. > I'm not sure why these extra restarts are needed? Couple restart added here mainly is to bypass high --query_log_write_interval_s=300. Remember that WorkloadManagementTestSuite will wait until impala-server.completed-queries.queued metric reach 0. http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_live.py@315 PS5, Line 315: self._start_impala_cluster(options=['--impalad_args=--executor_groups=extra ' > No need to set query_log_write_interval_s, see comment on line 79. This is a direct call to CustomClusterTestSuite._start_impala_cluster, and it does not aware about workload_mgmt=True, so this flag need to explicitly set. Remember that this patch also remove workload_mgmt arg from _start_impala_cluster. http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_log.py File tests/custom_cluster/test_query_log.py: http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_log.py@54 PS5, Line 54: cls.ImpalaTestMatrix.add_dimension(hs2_client_protocol_dimension()) > Since the client protocol recorded in the workload management tables change I am actually in favor of vector. See my explanation here: https://gerrit.cloudera.org/c/22617/5/tests/custom_cluster/test_workload_mgmt_sql_details.py#44 http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_log.py@996 PS5, Line 996: management is i > Nit: management is idle Done http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_workload_mgmt_init.py File tests/custom_cluster/test_workload_mgmt_init.py: http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_workload_mgmt_init.py@162 PS5, Line 162: workload_mgmt=True, > Note: --workload_mgmt_schema_version=1.1.0 should have been deleted because Lets tackle this in separate patch. http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_workload_mgmt_init.py@363 PS5, Line 363: additional_impalad_opts="--minidump_path={}".format(tmp_dir)) > Nit: the 4 lines aboce need to be indented two more spaces flake8 and autopep8 agree with this. autopep8 --ignore E121 --indent-size 2 --max-line-length 90 -i tests/custom_cluster/test_workload_mgmt_init.py In fact, autopep8 want to do even bigger code style fixes. Our python indentation enforcement is OK with either. So if gerrit critique does not complain, I usually let it slide. IMO, gerrit critique should change first to favor one over the other. See previous discussion at: https://gerrit.cloudera.org/c/22471/1/tests/conftest.py#410 https://gerrit.cloudera.org/c/22358/4/tests/query_test/test_kudu.py#173 http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_workload_mgmt_init.py@377 PS5, Line 377: QUERY_TBL_ALL)) > Nit: the 2 lines above need to be indented two more spaces flake8 and autopep8 agree with this. http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_workload_mgmt_sql_details.py File tests/custom_cluster/test_workload_mgmt_sql_details.py: http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_workload_mgmt_sql_details.py@44 PS5, Line 44: cls.ImpalaTestMatrix.add_dimension(hs2_client_protocol_dimension()) > Can this test also define `default_test_protocol()` and remove the use of v I can change, but I personally favor vector over default_test_protocol(). With vector, we can easily extend to test other protocol, say, hs2-http. We can easily declare constraint to not test certain protocol + other vector value combination. test_cancellation.py is an example of heavy vector & constraint declaration. IMO, if direct querying is preferred, we should stop using self.client and use self.hs2_client or self.beeswax_client instead, for better identification. I added default_test_protocol() mainly to make it easy to temporarily switch self.client type (from beeswax to hs2) for test that already use it pervasively (see IMPALA-13672). My goal is to switch default return value of default_test_protocol() to HS2 in the future. https://github.com/apache/impala/blob/efc921a83cf74c099e0c3d2d6a84f666dacabfa4/tests/common/base_test_suite.py#L43 This test is good, that it use vector from the beginning instead of self.client directly. -- To view, visit http://gerrit.cloudera.org:8080/22617 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iecf6452fa963304e263805ebeb017c843d17dd16 Gerrit-Change-Number: 22617 Gerrit-PatchSet: 6 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Fri, 14 Mar 2025 17:51:01 +0000 Gerrit-HasComments: Yes
