Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/23763 )
Change subject: IMPALA-14605: Fix memory leak in global admissiond for cancelled queued queries ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/23763/2/be/src/scheduling/admission-control-service.cc File be/src/scheduling/admission-control-service.cc: http://gerrit.cloudera.org:8080/#/c/23763/2/be/src/scheduling/admission-control-service.cc@410 PS2, Line 410: cleanup_queue_cv_.notify_one(); Better to use .notify_all() in case the is a future need to have multiple waiters on cleanup_queue_cv_ http://gerrit.cloudera.org:8080/#/c/23763/2/be/src/scheduling/admission-control-service.cc@420 PS2, Line 420: } If the ability to define a predicate on .wait() is used, then shutdown_ no longer needs to be an atomic_bool. Note that setting shutdown_ in AdmissionControlService::Join() will require locking the admission_state_map_ mutex. Also, I think lock can be defined outside of the while loop. Something like this: std::unique_lock<std::mutex> lock(cleanup_queue_lock_); while(true) { UniqueIdPB query_id; cleanup_queue_cv_.wait(lock, [&admission_state_cleanup_queue_, &shutdown] { return !admission_state_cleanup_queue_.empty() || shutdown; }); // At thus point, lock will be locked, loop through and dequeue/delete all entries in admission_state_map_. } http://gerrit.cloudera.org:8080/#/c/23763/2/be/src/scheduling/admission-control-service.cc@427 PS2, Line 427: UpdateAdmissionStateMapSizeMetric(); In change https://gerrit.cloudera.org/c/23763/, I added comments about avoiding calling this function. Please look at that change for details to see if they are applicable here too. http://gerrit.cloudera.org:8080/#/c/23763/2/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/23763/2/tests/custom_cluster/test_admission_controller.py@2468 PS2, Line 2468: quereis Nit: queries http://gerrit.cloudera.org:8080/#/c/23763/2/tests/custom_cluster/test_admission_controller.py@2479 PS2, Line 2479: sleep(5) Is there a way to wait for a certain metric value or a certain log message instead of sleeping an arbitrary number of seconds? Possibly by wrapping the below self.assert_log_contains_multiline with self.assert_eventually -- To view, visit http://gerrit.cloudera.org:8080/23763 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f16f3cf986e2038d6486d3ec687135d561e2cbf Gerrit-Change-Number: 23763 Gerrit-PatchSet: 2 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Fri, 12 Dec 2025 00:37:19 +0000 Gerrit-HasComments: Yes
