Jason Fehr 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 13: (4 comments) 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? This function does not use any private variable/functions of ClusterMembershipMgr thus I made it static to reduce the size of the ClusterMembershipMgr class. Would it be better as a private function on the ClusterMembershipMgr class? If so, I will definitely make the switch. 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? The "ADMISSIOND_TEST" environment variable is available as on option in some builds to enable running the admission control tests in a separate, standalone daemon without having to enable exhaustive tests (see TestAdmissionControllerWithACService and TestAdmissionControllerStressWithACService defined in this file). I attempted to use that option in a build, but it did nothing. After investigation, I found the "ADMISSIOND_TEST" environment variable was not handled anywhere, and thus I added it. 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') : result = self.execute_query_using_vector(query, vector) : assert result.success : > If you wait for https://gerrit.cloudera.org/c/22404 , there will be new me Done http://gerrit.cloudera.org:8080/#/c/22249/12/tests/custom_cluster/test_admission_controller.py@1840 PS12, Line 1840: @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) : def test_coord_only_pool_happy_path(self, > 4 test methods share the same start args. Is it better to group them in com I wish I could, but each test modifies the cluster daemons (for example, gracefully quiescing a coordinator, killing a coordinator, and adding a coordinator). Thus, each test needs a fresh cluster. -- 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: 13 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: Fri, 07 Feb 2025 21:14:36 +0000 Gerrit-HasComments: Yes
