Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/23760 )
Change subject: IMPALA-14612: Add global metrics for admission state map size ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.h File be/src/scheduling/admission-control-service.h: http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.h@134 PS1, Line 134: if (admission_state_map_size_ != nullptr) { Since admission_state_map_size_ is initialized in the class constructor, this nullptr check should not be necessary. If it is necessary, please wrap it with LIKELY() or replaced the if statement with a DCHECK. http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc File be/src/scheduling/admission-control-service.cc: http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc@157 PS1, Line 157: UpdateAdmissionStateMapSizeMetric(); In this situation, since we know only 1 query is being added, it would be better to directly call admission_state_map_size_->Increment(1) to avoid doing any locking in the admission_state_map_. http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc@228 PS1, Line 228: UpdateAdmissionStateMapSizeMetric(); In this situation, since we know only 1 query is being removed, it would be better to directly call admission_state_map_size_->Increment(-1) to avoid doing any locking in the admission_state_map_. http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc@253 PS1, Line 253: UpdateAdmissionStateMapSizeMetric(); In this situation, since we know only 1 query is being removed, it would be better to directly call admission_state_map_size_->Increment(-1) to avoid doing any locking in the admission_state_map_. http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc@318 PS1, Line 318: if (!cleaned_up.empty()) UpdateAdmissionStateMapSizeMetric(); Can the above for loop directly call admission_state_map_size_->Increment(-1) to avoid doing any locking in the admission_state_map_? Depending on how many queries are expected to be cleaned up, it may be better to update the metric once. In that case, please if declaring an int to track the number of deleted queries can passing that to admission_state_map_size_->Increment() is better than going through locking the admission_state_map_. http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc@337 PS1, Line 337: if (!cleaned_up.empty()) UpdateAdmissionStateMapSizeMetric(); Can the above for loop directly call admission_state_map_size_->Increment(-1) to avoid doing any locking in the admission_state_map_? Depending on how many queries are expected to be cleaned up, it may be better to update the metric once. In that case, please if declaring an int to track the number of deleted queries can passing that to admission_state_map_size_->Increment() is better than going through locking the admission_state_map_. http://gerrit.cloudera.org:8080/#/c/23760/1/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/23760/1/tests/custom_cluster/test_admission_controller.py@2392 PS1, Line 2392: assert old_admission_state_map_sz > 0 Can this assert be made more accurate? Possibly assert old_admission_state_map_sz == 0 http://gerrit.cloudera.org:8080/#/c/23760/1/tests/custom_cluster/test_admission_controller.py@2423 PS1, Line 2423: sleep(2) Instead of sleeping, use self.cluster.admissiond().service.wait_for_metric_value(). -- To view, visit http://gerrit.cloudera.org:8080/23760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie803aabf8d91b6381c5d0d7534cd9c9fc2166a73 Gerrit-Change-Number: 23760 Gerrit-PatchSet: 1 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Thu, 11 Dec 2025 23:49:25 +0000 Gerrit-HasComments: Yes
