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 15: (10 comments) http://gerrit.cloudera.org:8080/#/c/22249/15/be/src/scheduling/cluster-membership-mgr-test.cc File be/src/scheduling/cluster-membership-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/22249/15/be/src/scheduling/cluster-membership-mgr-test.cc@308 PS15, Line 308: UpdaUpdateMembership nit: fix this? http://gerrit.cloudera.org:8080/#/c/22249/15/be/src/scheduling/cluster-membership-mgr-test.cc@339 PS15, Line 339: ASSERT_EQ(1, cmm1.GetSnapshot()->current_backends.size()); : _assertCoords(cmm1, {"host_1"}); Can you unify the _assertCoords and current_backends.size() assertion? 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 inline void _removeCoordI > True, that is a good optimization. This function only gets called when a c Done http://gerrit.cloudera.org:8080/#/c/22249/15/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/22249/15/be/src/scheduling/cluster-membership-mgr.cc@173 PS15, Line 173: const BackendDescriptorPB* be Can this stay as constant reference instead of constant pointer? http://gerrit.cloudera.org:8080/#/c/22249/15/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/22249/15/be/src/util/webserver.cc@717 PS15, Line 717: LOG(INFO) << "Invalid JWT token provided"; Why 'bearer_token' is not printed anymore? http://gerrit.cloudera.org:8080/#/c/22249/15/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/22249/15/bin/start-impala-cluster.py@1149 PS15, Line 1149: elif options.add_impalads: : cluster_ops.start_impalads(options.num_coordinators, options.num_coordinators, Why this is changed? Would you like to have --add_coordinators argument instead? http://gerrit.cloudera.org:8080/#/c/22249/15/tests/common/custom_cluster_test_suite.py File tests/common/custom_cluster_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22249/15/tests/common/custom_cluster_test_suite.py@417 PS15, Line 417: catalog_log = self.assert_log_contains_multiline("catalogd", "INFO", r'Completed ' : r'workload management initialization.*?A catalog update with \d+ entries is ' : r'assembled\. Catalog version: (\d+)', timeout_s) What is the benefit of assert_log_contains_multiline compared to calling assert_catalogd_log_contains twice? They are practically the same. http://gerrit.cloudera.org:8080/#/c/22249/15/tests/common/custom_cluster_test_suite.py@563 PS15, Line 563: : if workload_mgmt: : cmd.append("--impalad_args=--enable_workload_mgmt=true") : cmd.append("--catalogd_args=--enable_workload_mgmt=true") Are you planning to use this new argument for other pre-existing tests? $ git grep enable_workload_mgmt tests/ | cut -d':' -f1 | sort | uniq -c 24 tests/custom_cluster/test_query_live.py 42 tests/custom_cluster/test_query_log.py 2 tests/custom_cluster/test_sys_db.py 46 tests/custom_cluster/test_workload_mgmt_init.py 22 tests/custom_cluster/test_workload_mgmt_sql_details.py http://gerrit.cloudera.org:8080/#/c/22249/15/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/22249/15/tests/custom_cluster/test_admission_controller.py@151 PS15, Line 151: RUN_ADMISSIOND_VAR = "ADMISSIOND_TEST" Please add this as py.test argument in conftest.py instead. DEFAULT_USE_ADMISSIOND = (os.getenv('ADMISSIOND_TEST', 'false') == 'true') ... parser.addoption("--use_admissiond", action="store_true", default=DEFAULT_USE_ADMISSIOND, help="start AdmissionD service for running tests") In turn, you can check the argument selection through pytest.config.option.use_admissiond, which I think should happen in CustomClusterTestSuite.setup_method(). http://gerrit.cloudera.org:8080/#/c/22249/15/tests/custom_cluster/test_admission_controller.py@2807 PS15, Line 2807: if not (self.exploration_strategy() == 'exhaustive' : or os.getenv(RUN_ADMISSIOND_VAR, 'false').strip().lower() == 'true'): : pytest.skip("runs only in exhaustive or if '{}' environment variable is set to " : "'true'".format(RUN_ADMISSIOND_VAR)) Nowhere in our infra script that we set 'export ADMISSIOND_TEST=true'. That means, we'll never execute TestAdmissionControllerStressWithACService and TestAdmissionControllerWithACService this way. -- 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: 15 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: Mon, 10 Feb 2025 20:19:12 +0000 Gerrit-HasComments: Yes
