Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24129 )
Change subject: IMPALA-14840: Enforce pool Max Memory when query limits are unset ...................................................................... Patch Set 3: (6 comments) Sorry for the late reply, thanks for the comments! 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: Enforce pool Max Memory when query limits are unset > nit: The commit message subject is a bit long (98 characters). We only enforce the 72-char limit on the body, title can be longer. But I agree that the proposed title is better. http://gerrit.cloudera.org:8080/#/c/24129/2//COMMIT_MSG@20 PS2, Line 20: The patch also removes the unused 'root_cfg' parameter of : AdmissionController::CanAdmitRequest(). > nit: Explicitly naming the removed parameter (root_cfg) might be better, so Done 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: ds), ensur > nit: The inline comment // > 2 GB / 2 is mathematically correct but a bit Done 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: no m > small nit: The comment here originally said "(If both these max/min limits Reworded a bit. 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 && > nit: Maybe aligning the boolean conditions here might be better for visual Done 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 Thanks! :) -- 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: 3 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: Thu, 09 Apr 2026 15:41:24 +0000 Gerrit-HasComments: Yes
