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 11:

(10 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.
> I imagine that the number of users that can concurrently have queries in an
And just to double check, the username here is simply 'user', not 
'user/instance@REALM' from kerberos, correct?
https://gerrit.cloudera.org/c/21925/ for context.


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;
> First of all, if user quotas are not configured then there is only a trivia
Ack


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",
> I found it useful for debugging to see the list, plus it provides some obse
Ack


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 static byte[] getHadoopGroupsInternal(byte[] 
serializedRequest)
> Done
Done


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.Test;
             : import org.junit.rules.TemporaryFolder;
             :
             : import static java.util.Arrays.asList;
             : import static 
org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_SECURITY_AUTH_TO_LOCAL;
             : import static 
org.apache.impala.yarn.server.resourcemanager.scheduler.fair.
             :     AllocationFileLoaderService.addQueryLimits;
             :
             : import org.apache.hadoop.conf.Configuration;
             : import org.apache.hadoop.util.StringUtils;
             :
> OK I redid the imports to avoid clang-format's reorderng
Done


http://gerrit.cloudera.org:8080/#/c/21616/10/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java@207
PS10, Line 207: String expectedMessage = "Failed to resolve user 'userA' to a 
pool
> Done, assuming I understand what you mean
Done


http://gerrit.cloudera.org:8080/#/c/21616/11/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/11/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java@225
PS11, Line 225: teh
nit: the


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>
> When there are limits placed at the root then the quotas are enforced acros
I see.. thank you for the explanation.


http://gerrit.cloudera.org:8080/#/c/21616/10/fe/src/test/resources/fair-scheduler-test.xml@27
PS10, Line 27:     <queue name="queueD">
> The llama.am.throttling.maximum.placed.reservations.* rules are enforced. T
Ack. Lets document this later that there can be two different limit in two 
different config files.


http://gerrit.cloudera.org:8080/#/c/21616/10/fe/src/test/resources/fair-scheduler-test.xml@28
PS10, Line 28: userA,userB
> If I read the code right, this aclSubmitApps is parsed using Hadoop code ht
Ok, I read it wrong then. Thank you for pointing the difference between comma 
and space delimiter.



--
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: 11
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: Tue, 15 Oct 2024 17:02:42 +0000
Gerrit-HasComments: Yes

Reply via email to