impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Date Tue, 19 Sep 2017 01:15:40 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
......................................................................


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java
File common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java:

Line 64:   private final Map<String, Map<QueueACL, AccessControlList>> queueAcls;
> I don't think it's worth changing this one otherwise I'll have to modify a 
That's fine.  We may even want extra ACLs sometime in the future - I can see a case for CANCEL_OTHERS


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java
File common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java:

Line 30: public class AllocationConfigurationException extends Exception {
> We still use the resource parsing code that throws this, e.g. in FairSchedu
Fair enough.


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java
File common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java:

Line 103:   public void serviceInit(Configuration conf) throws Exception {
> Yeah you're right, though I'm on the fence about how much we should doctor 
Can you file a JIRA?  This is actually a well compartmentalized ramp-up or beginner task (that
even someone from outside Cloudera could do)


Line 223:     Map<String, Long> minSharePreemptionTimeouts = new HashMap<>();
> I think it may be worth leaving this code as-is even if it's doing extra wo
Seems fine.


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java
File common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java:

Line 70:     Pattern pattern = Pattern.compile("(\\d+)(\\.\\d*)?\\s*" + units);
> Agreed, though I don't think it's worth spending time improving this code. 
Nope.  I have zero insight into how often this is called.  Obviously if it ends up being called
every time we need to check a memory reservation it might be a problem, but that seems very
unlikely.


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java
File common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java:

Line 57:   
Is it worth fixing the trailing spaces so future diffs don't give the linter hives?


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java
File common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java:

Line 25:     return DEFAULT_POLICY;
> Per my other comments, I'd prefer not to change the parsing code too much y
Seems fine.


http://gerrit.cloudera.org:8080/#/c/8035/4/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java
File fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java:

Line 124:         new Configuration().get(HADOOP_SECURITY_AUTH_TO_LOCAL, "DEFAULT"));
> I'm actually not sure why the testResolvePrincipalName() was working before
If I had to guess, I'd guess that kerberos tests were flaky and got disabled or marked xfailed.


-- 
To view, visit http://gerrit.cloudera.org:8080/8035
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mjacobs@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mjacobs@apache.org>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message