Andrew Sherman 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: (6 comments) Thanks for the review 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: int64_t increment(const std::string& key); > nit: can we make increment mirror decrement and also return the new value? Done 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@829 PS3, Line 829: }; > Removing private here seems like a mistake. Ah you are right, thanks 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()) { > Let's add a comment trying to describe the relation. Done 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: private static final Configuration CONF = new Configuration(); > I don't see a call to getHadoopconfig. Is that a layer down somewhere? Yes this was copy and paste without thought. I'm just deleting this comment. 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 snippet of xml, and then call addQueryLimits() on the root element. > typo: snippet Done 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: <user> userG </user> > Is this meant to test some of the XML parsing with extra spaces? yes, added a comment to clarify. -- 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: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Mon, 23 Sep 2024 20:44:42 +0000 Gerrit-HasComments: Yes
