Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21616 )
Change subject: IMPALA-12345: Add user quotas to Admission Control ...................................................................... Patch Set 7: (22 comments) http://gerrit.cloudera.org:8080/#/c/21616/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21616/3//COMMIT_MSG@24 PS3, Line 24: two reasons: (1) queries can only be admitted from the front of the > Done Ack http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/common/global-flags.cc@193 PS3, Line 193: #ifdef SLOW_BUILD > Done Ack http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller-test.cc@404 PS3, Line 404: EXPECT_FALSE(false) << "unknown outcome"; > Done I was surprised that we need a return, but based on https://google.github.io/googletest/advanced.html#assertion-placement looks like there aren't any other options. http://gerrit.cloudera.org:8080/#/c/21616/7/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/21616/7/be/src/scheduling/admission-controller.h@558 PS7, Line 558: void increment(const std::string& key); nit: can we make increment mirror decrement and also return the new value? http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.h@535 PS3, Line 535: typedef std::map<std::string, int64_t> UserLoads; > Thanks, done Ack http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.h@829 PS3, Line 829: // Return a string reporting top 5 queries with most memory consumed among all Removing private here seems like a mistake. http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@770 PS3, Line 770: if (!user.empty()) { > The idea is that RunningQuery.query is set only if user quotas are configur Let's add a comment trying to describe the relation. http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@861 PS3, Line 861: for (const auto& load : loads) { > Thanks, yes, original code is too cautious Done http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@882 PS3, Line 882: } > Thanks Done http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1161 PS3, Line 1161: return true; > Good advice, thanks Ack http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1177 PS3, Line 1177: quota_exceeded_reason, &key_matched)) { > Done Ack http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1205 PS3, Line 1205: user_limit = it->second; > Done Ack http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1213 PS3, Line 1213: *key_matched = true; > Done Ack http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1611 PS3, Line 1611: > This was to make clion shut up. I'll restore the old code Ack http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1940 PS3, Line 1940: void AdmissionController::UpdatePoolStats( > Done Ack http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@2190 PS3, Line 2190: // process executor groups in alphanumerically sorted order. > Yeah ask clang-format Ack http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@2418 PS3, Line 2418: ClusterMembershipMgr::SnapshotPtr membership_snapshot = > Yes, separated out so that unit tests could call TryDequeue() to make a sin Ack http://gerrit.cloudera.org:8080/#/c/21616/7/fe/src/main/java/org/apache/impala/util/JniRequestPoolService.java File fe/src/main/java/org/apache/impala/util/JniRequestPoolService.java: http://gerrit.cloudera.org:8080/#/c/21616/7/fe/src/main/java/org/apache/impala/util/JniRequestPoolService.java@62 PS7, Line 62: // Caching this saves ~50ms per call to getHadoopConfig I don't see a call to getHadoopconfig. Is that a layer down somewhere? http://gerrit.cloudera.org:8080/#/c/21616/7/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java File fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java: http://gerrit.cloudera.org:8080/#/c/21616/7/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java@390 PS7, Line 390: * Parse a snipped of xml, and then call addQueryLimits() on the root element. typo: snippet 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> > Ha the answer is because that was easy in the Java code. Ack http://gerrit.cloudera.org:8080/#/c/21616/7/fe/src/test/resources/fair-scheduler-test.xml File fe/src/test/resources/fair-scheduler-test.xml: http://gerrit.cloudera.org:8080/#/c/21616/7/fe/src/test/resources/fair-scheduler-test.xml@40 PS7, Line 40: <limit> 101 </limit> Is this meant to test some of the XML parsing with extra spaces? http://gerrit.cloudera.org:8080/#/c/21616/3/java/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java File java/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java: http://gerrit.cloudera.org:8080/#/c/21616/3/java/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java@21 PS3, Line 21: import java.util.Collections; > I think we've forked this code, so we own it. Ack -- 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: 7 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Tue, 03 Sep 2024 23:43:53 +0000 Gerrit-HasComments: Yes
