Surya Hebbar has posted comments on this change. ( http://gerrit.cloudera.org:8080/24129 )
Change subject: IMPALA-14840: Pool can use more memory than Max Memory when Min/Max Query Memory limit is not set ...................................................................... Patch Set 2: Code-Review+1 (6 comments) Thank your for the patch. A very clean fix for the resource leakage! I've left a few minor nits inline mostly regarding comment clarity and commit message formatting. Overall, the logic looks solid. LGTM! http://gerrit.cloudera.org:8080/#/c/24129/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24129/2//COMMIT_MSG@7 PS2, Line 7: IMPALA-14840: Pool can use more memory than Max Memory when Min/Max Query Memory limit is not set nit: The commit message subject is a bit long (98 characters). Please consider shortening it to fit standard git log formatting. e.g. IMPALA-14840: Enforce pool Max Memory when query limits are unset http://gerrit.cloudera.org:8080/#/c/24129/2//COMMIT_MSG@20 PS2, Line 20: The patch also removes an unused parameter of : AdmissionController::CanAdmitRequest(). nit: Explicitly naming the removed parameter (root_cfg) might be better, so it's easily searchable in the git history: "The patch also removes the unused root_cfg parameter of AdmissionController::CanAdmitRequest()." http://gerrit.cloudera.org:8080/#/c/24129/2/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/24129/2/be/src/scheduling/admission-controller-test.cc@2288 PS2, Line 2288: > 2 GB / 2 nit: The inline comment // > 2 GB / 2 is mathematically correct but a bit brief. Can it be expanded slightly so the reader immediately understands the constraint we are testing against? Maybe something like the following, // 3 GB is > (pool_max_mem / num_backends), ensuring the estimate exceeds the per-node pool budget http://gerrit.cloudera.org:8080/#/c/24129/2/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/24129/2/be/src/scheduling/admission-controller.h@193 PS2, Line 193: both small nit: The comment here originally said "(If both these max/min limits are not configured...)". Since we are now checking three properties (min_query_mem_limit, max_query_mem_limit, and max_mem_resources), consider removing the word "both". ... (If these max/min limits are not configured, and there is no max_mem_resources configured for the pool, then...) http://gerrit.cloudera.org:8080/#/c/24129/2/be/src/scheduling/schedule-state.cc File be/src/scheduling/schedule-state.cc: http://gerrit.cloudera.org:8080/#/c/24129/2/be/src/scheduling/schedule-state.cc@305 PS2, Line 305: const bool mimic_old_behaviour = : pool_cfg.max_mem_resources <= 0 && : pool_cfg.min_query_mem_limit == 0 && pool_cfg.max_query_mem_limit == 0; nit: Maybe aligning the boolean conditions here might be better for visual flow, or wrapping them if they exceed the column limit. const bool mimic_old_behaviour = pool_cfg.max_mem_resources <= 0 && pool_cfg.min_query_mem_limit == 0 && pool_cfg.max_query_mem_limit == 0; http://gerrit.cloudera.org:8080/#/c/24129/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/24129/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@809 PS2, Line 809: Math.max(numExecutorsForPlanning(), 1) Great catch of numExecutorsForPlanning() division-by-zero in planning phase! -- To view, visit http://gerrit.cloudera.org:8080/24129 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4834964e4361895e10627a661831253ce676c129 Gerrit-Change-Number: 24129 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 27 Mar 2026 20:47:44 +0000 Gerrit-HasComments: Yes
