Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22617 )
Change subject: IMPALA-13861: Standardize workload management tests ...................................................................... Patch Set 5: (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: system I think "workload management" instead of "system" would be clearer here. The reason is only the sys.impala_query_live table is a system table. 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 minicluster instance for all tests. http://gerrit.cloudera.org:8080/#/c/22617/5//COMMIT_MSG@40 PS5, Line 40: enable Nit: enables http://gerrit.cloudera.org:8080/#/c/22617/5//COMMIT_MSG@43 PS5, Line 43: change Nit: changes 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 http://gerrit.cloudera.org:8080/#/c/22617/5/tests/common/wm_test_suite.py@26 PS5, Line 26: wait nit: waits http://gerrit.cloudera.org:8080/#/c/22617/5/tests/common/wm_test_suite.py@27 PS5, Line 27: reach nit: reaches 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 workload_mgmt is True, we do need to explicitly set --query_log_write_interval_s to some large number on all these admission controller tests to ensure the workload management insert dml does not run. 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 workload_mgmt is True, we do need to explicitly set --query_log_write_interval_s to some large number on all the tests in this file to ensure the workload management insert dml does not run. http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_live.py@189 PS5, Line 189: self.cluster.impalads[0].restart(SIGRTMIN) I'm not sure why this extra restart is needed since the `drop table` dml isn't asserted to be in the sys.impala_query_log table. http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_live.py@303 PS5, Line 303: self.cluster.impalads[1].restart(SIGRTMIN) I'm not sure why these extra restarts are needed? http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_live.py@315 PS5, Line 315: '--query_log_write_interval_s=1 ' No need to set query_log_write_interval_s, see comment on line 79. 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 changes based on beeswax versus hs2, it makes sense to have a single beeswax test to verify this functionality. Please also consider modifying the other tests to use HS2 directly like test_query_live.py (but I understand if that would complicate this change too much). http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_log.py@996 PS5, Line 996: management idle Nit: management is idle 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 it was not necessary to the test and also was a misconfiguration since impalad was missing the flag. 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 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 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 vector? -- 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: 5 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Thu, 13 Mar 2025 20:10:44 +0000 Gerrit-HasComments: Yes
