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 <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
