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 10: (9 comments) http://gerrit.cloudera.org:8080/#/c/21616/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21616/10//COMMIT_MSG@17 PS10, Line 17: TPoolStats (the data that is shared between Admission Control instances) : is extended to include a map from user name to a count of queries : running. How scaleable is this map if there are hundreds of users active? Any memory implication? http://gerrit.cloudera.org:8080/#/c/21616/10/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/21616/10/be/src/scheduling/admission-controller.h@586 PS10, Line 586: /// Export user names to a metrics set. : void export_users(SetMetric<std::string>* metrics) const; Is this scalable if there are hundreds of users? http://gerrit.cloudera.org:8080/#/c/21616/10/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/21616/10/common/thrift/metrics.json@293 PS10, Line 293: "description": "Resource Pool $0 Aggregate Users", Is it important to show all currently active user names? Is displaying distinct count sufficient? http://gerrit.cloudera.org:8080/#/c/21616/10/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/10/fe/src/main/java/org/apache/impala/util/JniRequestPoolService.java@135 PS10, Line 135: public byte[] getHadoopGroups(byte[] serializedRequest) throws ImpalaException { This is duplicate with exact code in JniFrontend.java Can they turned into static method and shared? http://gerrit.cloudera.org:8080/#/c/21616/10/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/10/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java@41 PS10, Line 41: import org.junit.After; : import org.junit.AfterClass; : import org.junit.Assert; : import org.junit.BeforeClass; : import org.junit.Ignore; : import org.junit.Rule; : import org.junit.Test; : import org.junit.rules.TemporaryFolder; : import org.w3c.dom.Document; : import org.w3c.dom.Element; : import org.xml.sax.SAXException; Unnecessary import reordering? http://gerrit.cloudera.org:8080/#/c/21616/10/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java@207 PS10, Line 207: Assert.assertTrue(poolService_.hasAccess("root.queueA", "userA")); nit: I think existing tests that verify poolService_.hasAccess() can be improved by testing all positive and negative combinations. http://gerrit.cloudera.org:8080/#/c/21616/10/fe/src/test/resources/fair-scheduler-test.xml File fe/src/test/resources/fair-scheduler-test.xml: http://gerrit.cloudera.org:8080/#/c/21616/10/fe/src/test/resources/fair-scheduler-test.xml@3 PS10, Line 3: <queue name="root"> : <userQueryLimit> : <user>userB</user> : <totalCount>6</totalCount> : </userQueryLimit> : <userQueryLimit> : <user>*</user> : <totalCount>10</totalCount> : </userQueryLimit> : <groupQueryLimit> : <group>group3</group> : <totalCount>5</totalCount> : </groupQueryLimit> nit: Discussion. I wish that we don't need to support advanced quota accounting for non-default pool. To me, if admin want to enforce strict quota, then they need to move away from default pool completely to avoid confusion. They should just use simplistic "Max Running Queries" and "Max Queued Queries" by putting this in llama config llama.am.throttling.maximum.placed.reservations.root.default llama.am.throttling.maximum.queued.reservations.root.default http://gerrit.cloudera.org:8080/#/c/21616/10/fe/src/test/resources/fair-scheduler-test.xml@27 PS10, Line 27: <queue name="queueD"> What will happen if there is per-pool limit set in llama config? <property> <name>llama.am.throttling.maximum.placed.reservations.root.queueD</name> <value>1</value> </property> http://gerrit.cloudera.org:8080/#/c/21616/10/fe/src/test/resources/fair-scheduler-test.xml@28 PS10, Line 28: userA,userB This use comma delimiter, but L21 use space delimiter. I think we should be consistent in using space delimiter. >From FairScheduler.html docs: > Note: The delimiter is a space character. To specify only ACL groups, begin > the value with a space character. -- 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: 10 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: Fri, 27 Sep 2024 17:15:55 +0000 Gerrit-HasComments: Yes
