Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22617 )
Change subject: IMPALA-13861: Standardize workload management tests ...................................................................... Patch Set 6: (17 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 > Done 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 > Done Done http://gerrit.cloudera.org:8080/#/c/22617/5//COMMIT_MSG@40 PS5, Line 40: differ > Done Done http://gerrit.cloudera.org:8080/#/c/22617/5//COMMIT_MSG@43 PS5, Line 43: > Done 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 > Done Done http://gerrit.cloudera.org:8080/#/c/22617/5/tests/common/wm_test_suite.py@26 PS5, Line 26: wait > Done Done http://gerrit.cloudera.org:8080/#/c/22617/5/tests/common/wm_test_suite.py@27 PS5, Line 27: reach > Done 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( > I don't think it matters, because workload_mgmt enables graceful shutdown b Yeah, you are correct since the tests only use the profile from a query to sys.impala_query_live and not the actual results of queries to those tables. 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", > Correct. CustomClusterTestSuite._start_impala_cluster, however, is an excep There is still the potential for brittle tests. Take, for example, lines 87-89. The client executes a query and then immediately checks the sys.impala_query_live table for that query. If the workload management processing loop ran in between those two steps, then the test will fail since it won't find the query it just ran in the sys.impala_query_live table. I'm not sure of a good way to handle this though. Since the tests now wait for the workload management queue to drop to 0, the query_log_write_interval_s cannot be set to a high value without adding an extra restart. Since these tests are passing as-is, maybe just bumping query_log_write_interval_s to 2 will give enough time for the query against sys.impala_query_live to run before the workload management processing loop. 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. > Couple restart added here mainly is to bypass high --query_log_write_interv Ah, makes sense now. 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. > Couple restart added here mainly is to bypass high --query_log_write_interv Done 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 ' > This is a direct call to CustomClusterTestSuite._start_impala_cluster, and Yeah, the non-dedicated coordinator will need both those flags. I missed the comment on line 312 and was thinking this code only added executors. 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()) > I am actually in favor of vector. See my explanation here: Done http://gerrit.cloudera.org:8080/#/c/22617/5/tests/custom_cluster/test_query_log.py@996 PS5, Line 996: management is i > Done 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, > Lets tackle this in separate patch. Ack 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)) > flake8 and autopep8 agree with this. Interesting. We have a C++ style guide that lays out the 4 space requirement https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536#C++StyleGuide-Formatting but not a python style guide. Sounds like a bigger discussion to be had. Still I prefer to stick with existing styling, and that vast majority of the python code uses 4 spaces for line continuation. 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()) > I can change, but I personally favor vector over default_test_protocol(). Thanks for the explanation. I was asking because there is a difference in each test setup (one using vector and one not) and wanted to get on a single approach. I agree with your arguments why the vector approach is better. -- 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 <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: Fri, 14 Mar 2025 22:33:53 +0000 Gerrit-HasComments: Yes
