Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21616 )
Change subject: IMPALA-12345: Add user quotas to Admission Control ...................................................................... Patch Set 8: (5 comments) Will need more time to thoroughly review this patch. But here is my few early comment. http://gerrit.cloudera.org:8080/#/c/21616/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21616/8//COMMIT_MSG@37 PS8, Line 37: Two new elements ‘userQueryLimit’ and ‘groupQueryLimit’ can be added to : the fair-scheduler.xml file. These elements can be placed on the root : configuration, which applies to all pools, or the pool configuration. : The ‘userQueryLimit’ element has 2 child elements: "user" and "limit". : The 'user' element contains the short names of users, and can be : repeated, or have the value "*" for a wildcard name which matches all : users. The ‘groupQueryLimit’ element has 2 child elements: "group" : and "limit". The 'group' element contains group names. I recommend naming "totalCount" instead of "limit". I'm guessing that totalMemory and totalSlot / totalCpu will follow soon after this totalCount quota. If that is true, is current config structure easily extendable for such accounting? http://gerrit.cloudera.org:8080/#/c/21616/8/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/21616/8/be/src/scheduling/admission-controller.h@1152 PS8, Line 1152: void UpdateStatsOnAdmission(const ScheduleState& state, const std::string& user, : bool was_queued, bool is_trivial, bool track_per_user); Can user, was_queued, and track_per_user grouped into single optional struct? Please document new parameter as well. http://gerrit.cloudera.org:8080/#/c/21616/8/be/src/scheduling/admission-controller.h@1277 PS8, Line 1277: static bool HasSufficientUserQuota(const TPoolConfig& pool_cfg, const string& pool_name, Add the same comment as HasSufficientGroupQuota > /// When a rule is evaluated, and passed, then *key_matched is set to True. http://gerrit.cloudera.org:8080/#/c/21616/8/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/21616/8/be/src/scheduling/admission-controller.cc@747 PS8, Line 747: if (track_per_user && !was_queued) { : // If the query was not previously queued then track the user counts. : IncrementPerUser(user); : } Why there is a per-user tracking but no per-group tracking? http://gerrit.cloudera.org:8080/#/c/21616/3/fe/src/test/resources/fair-scheduler-test.xml File fe/src/test/resources/fair-scheduler-test.xml: http://gerrit.cloudera.org:8080/#/c/21616/3/fe/src/test/resources/fair-scheduler-test.xml@4 PS3, Line 4: <userQueryLimit> Thanks. I checked Impala docs and indeed find following sentence: > These files define resource pools for Impala admission control and are > separate from the similar fair-scheduler.xml that defines resource pools for > YARN. https://impala.apache.org/docs/build/html/topics/impala_admission_config.html In the future, we probably should replace both fair-scheduler.xml and llama-site.xml with system table like impala_query_log. -- To view, visit http://gerrit.cloudera.org:8080/21616 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c33f3f2427db57fb9b6c593a4b22d5029549b41 Gerrit-Change-Number: 21616 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Mon, 23 Sep 2024 23:09:20 +0000 Gerrit-HasComments: Yes
