Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22249 )
Change subject: IMPALA-13201: System Table Queries Execute When Admission Queues are Full ...................................................................... Patch Set 12: (6 comments) I need to do another pass on this, but here is my initial comments. http://gerrit.cloudera.org:8080/#/c/22249/7/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/22249/7/be/src/scheduling/admission-controller.cc@2271 PS7, Line 2271: > I modified the code so that all_coordinators is now a plain ExecutorGroup i Done http://gerrit.cloudera.org:8080/#/c/22249/7/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/22249/7/be/src/scheduling/cluster-membership-mgr.h@115 PS7, Line 115: ExecutorGroup all_coordinators > Done. all_coordinators is managed in ClusterMembershipMgr::UpdateMembershi Done http://gerrit.cloudera.org:8080/#/c/22249/12/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/22249/12/be/src/scheduling/cluster-membership-mgr.cc@192 PS12, Line 192: static void _removeCoordIfExists Why this is not private inline method under ClusterMembershipMgr? http://gerrit.cloudera.org:8080/#/c/22249/12/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/22249/12/tests/custom_cluster/test_admission_controller.py@149 PS12, Line 149: # Name of the environment variable that forces the tests for admission controller as a : # separate daemon to run. Set this variable to 'true' to run the additional tests. : RUN_ADMISSIOND_VAR = "ADMISSIOND_TEST" Why this is needed? Is it leftover from your local testing? http://gerrit.cloudera.org:8080/#/c/22249/12/tests/custom_cluster/test_admission_controller.py@1831 PS12, Line 1831: : vector.set_exec_option('request_pool', 'onlycoords') : client = self.default_impala_client(vector.get_value('protocol')) : result = self.execute_query_using_client(client, query, vector) : assert result.success If you wait for https://gerrit.cloudera.org/c/22404 , there will be new method execute_query_using_vector() that can slightly make this more concise. http://gerrit.cloudera.org:8080/#/c/22249/12/tests/custom_cluster/test_admission_controller.py@1840 PS12, Line 1840: @pytest.mark.execute_serially : @CustomClusterTestSuite.with_args(num_exclusive_coordinators=3, cluster_size=5, : workload_mgmt=True, impalad_args=impalad_admission_ctrl_config_args( : fs_allocation_file="fair-scheduler-onlycoords.xml", : llama_site_file="llama-site-onlycoords.xml"), : statestored_args=_STATESTORED_ARGS) 4 test methods share the same start args. Is it better to group them in common class and let them share cluster without restart in-between them? -- To view, visit http://gerrit.cloudera.org:8080/22249 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5e0e64db92bdbf80f8b5bd85d001ffe4c8c9ffda Gerrit-Change-Number: 22249 Gerrit-PatchSet: 12 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 06 Feb 2025 01:17:53 +0000 Gerrit-HasComments: Yes
