Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22329 )
Change subject: IMPALA-12494: Clamp MEM_LIMIT_EXECUTORS and MEM_LIMIT_COORDINATORS ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/22329/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22329/2//COMMIT_MSG@18 PS2, Line 18: ['min-query-mem-limit-coordinators', 'max-query-mem-limit-coordinators'] Is this new config? If yes, please explicitly mention it in the commit message. Are they added because we want to clamp MEM_LIMIT_COORDINATORS differently than MEM_LIMIT clamping? http://gerrit.cloudera.org:8080/#/c/22329/2/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/22329/2/be/src/scheduling/admission-controller-test.cc@1961 PS2, Line 1961: ScheduleState* schedule_state = : DedicatedCoordAdmissionSetup(pool_config, -1, 3 * GIGABYTE, -1, false); I already forgot what those parameters are. Can you please arranging them with comments like this: ScheduleState* schedule_state = DedicatedCoordAdmissionSetup(pool_config, /* mem_limit */ -1, /* mem_limit_executors */ 3 * GIGABYTE, /* mem_limit_coordinators */ -1, /* use_default_pool_config */ -1); http://gerrit.cloudera.org:8080/#/c/22329/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/22329/2/be/src/scheduling/admission-controller.cc@1020 PS2, Line 1020: impala.admission-control.max-query-mem-limit Please document that impala.admission-control.max-query-mem-limit applies for both MEM_LIMIT and MEM_LIMIT_EXECUTORS. docs/topics/impala_admission.xml is an apropriate place to mention those options. There is a mention of impala.admission-control.max-query-mem-limit there. For option that so intertwined with pool config and other options, I wish there are docs for MEM_LIMIT_EXECUTOR and MEM_LIMIT_COORDINATORS, but I can only find ./docs/topics/impala_mem_limit.xml. Can you please add docs for both of them? http://gerrit.cloudera.org:8080/#/c/22329/2/be/src/scheduling/admission-controller.cc@1440 PS2, Line 1440: REASON_INVALID_POOL_CONFIG_MIN_LIMIT_COORD_MAX_LIMIT_COORD, : PrintBytes(pool_cfg.min_query_mem_limit_c maybe wrap them with PrintBytes() here and others above? http://gerrit.cloudera.org:8080/#/c/22329/2/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/22329/2/common/thrift/ImpalaInternalService.thrift@239 PS2, Line 239: 14: required i64 max_query_mem_limit_coordinators = 0; > Adding a required field 'max_query_mem_limit_coordinators' in TPoolConfig m Given the warning, I think this should be made optional. It will impact zero-down-time upgrade capabilities. Quanlong/Wenzhe might have opinion on this. http://gerrit.cloudera.org:8080/#/c/22329/2/fe/src/test/resources/mem-limit-test-fair-scheduler.xml File fe/src/test/resources/mem-limit-test-fair-scheduler.xml: http://gerrit.cloudera.org:8080/#/c/22329/2/fe/src/test/resources/mem-limit-test-fair-scheduler.xml@40 PS2, Line 40: memLimitCoordExecClamp I suggest to rename to poolWithCoordAndExecLimitClamp for clarity. -- To view, visit http://gerrit.cloudera.org:8080/22329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I262910284cbf84b8ba7e0c949bbf053dbfe44a0d Gerrit-Change-Number: 22329 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Sat, 11 Jan 2025 05:43:41 +0000 Gerrit-HasComments: Yes
