Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22443 )
Change subject: IMPALA-13726 Add admission control slots to /queries page in webui ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/22443/2/be/src/service/query-state-record-test.cc File be/src/service/query-state-record-test.cc: http://gerrit.cloudera.org:8080/#/c/22443/2/be/src/service/query-state-record-test.cc@170 PS2, Line 170: 7 Nit: I suggest making this value '8' to assert the get_admission_slots function is returning the first executor. http://gerrit.cloudera.org:8080/#/c/22443/2/testdata/workloads/functional-query/queries/QueryTest/workload-mgmt-impala_query_live-v1.2.0.test File testdata/workloads/functional-query/queries/QueryTest/workload-mgmt-impala_query_live-v1.2.0.test: http://gerrit.cloudera.org:8080/#/c/22443/2/testdata/workloads/functional-query/queries/QueryTest/workload-mgmt-impala_query_live-v1.2.0.test@58 PS2, Line 58: 'orderby_columns','string','' This test result list is missing coordinator_slots and executor_slots. http://gerrit.cloudera.org:8080/#/c/22443/2/testdata/workloads/functional-query/queries/QueryTest/workload-mgmt-impala_query_log-v1.2.0.test File testdata/workloads/functional-query/queries/QueryTest/workload-mgmt-impala_query_log-v1.2.0.test: http://gerrit.cloudera.org:8080/#/c/22443/2/testdata/workloads/functional-query/queries/QueryTest/workload-mgmt-impala_query_log-v1.2.0.test@58 PS2, Line 58: 'orderby_columns','string',NULL This test result list is missing coordinator_slots and executor_slots. http://gerrit.cloudera.org:8080/#/c/22443/2/tests/custom_cluster/test_workload_mgmt_init.py File tests/custom_cluster/test_workload_mgmt_init.py: http://gerrit.cloudera.org:8080/#/c/22443/2/tests/custom_cluster/test_workload_mgmt_init.py@176 PS2, Line 176: 1.0.0 Nit: should be "1.1.0" http://gerrit.cloudera.org:8080/#/c/22443/2/tests/custom_cluster/test_workload_mgmt_init.py@188 PS2, Line 188: self.LATEST_SCHEMA I would prefer to pass in the string "1.2.0" here only because modifying the LATEST_SCHEMA constant will cause this test name to mismatch the actual version its testing. http://gerrit.cloudera.org:8080/#/c/22443/2/tests/custom_cluster/test_workload_mgmt_init.py@200 PS2, Line 200: # Verify the initial table create on version 1.0.0 succeeded. Nit: unneessary outdent http://gerrit.cloudera.org:8080/#/c/22443/2/tests/custom_cluster/test_workload_mgmt_init.py@220 PS2, Line 220: def test_upgrade_1_1_0_to_1_2_0(self, vector): Please add an additional test for upgrading from 1.0.0 to 1.2.0. http://gerrit.cloudera.org:8080/#/c/22443/2/tests/custom_cluster/test_workload_mgmt_init.py@257 PS2, Line 257: # Run a query and ensure it does not populate version 1.2.0 fields. Nit: should say something like "does not populate fields other than version 1.0.0 fields. http://gerrit.cloudera.org:8080/#/c/22443/2/tests/util/workload_management.py File tests/util/workload_management.py: http://gerrit.cloudera.org:8080/#/c/22443/2/tests/util/workload_management.py@618 PS2, Line 618: expected_executor_slots = "0" Please consider moving the calculation of expected_coordinator_slots and expected_executor_slots to the else clauses of the if statements below. I suspect it will make the code a bit clearer. -- To view, visit http://gerrit.cloudera.org:8080/22443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I057493b7767902a417dfeb75cdaeffd452d66789 Gerrit-Change-Number: 22443 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Mon, 03 Feb 2025 22:35:33 +0000 Gerrit-HasComments: Yes
