Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar ......................................................................
Patch Set 4: (13 comments) Thanks, Zach - some good observations. I made a bunch of the changes for additional cleanup that you suggested, but some I held off on because I don't wanna make diffing the more complex classes harder later. We can always choose to simplify this further later, if we want. Let me know if you feel strongly about it. 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 61: private final float queueMaxAMShareDefault; > Impala doesn't make use of YARN's concept of ApplicationMasters. Why not k Done Line 64: private final Map<String, Map<QueueACL, AccessControlList>> queueAcls; > Useless (just SUBMIT_APPLICATIONS) I don't think it's worth changing this one otherwise I'll have to modify a bunch of code that I don't wanna bother adding extra tests for. Line 69: private final Map<String, Long> minSharePreemptionTimeouts; > Unused by Impala Done Line 74: private final Map<String, Long> fairSharePreemptionTimeouts; > Also unused by Impala Done Line 80: private final Map<String, Float> fairSharePreemptionThresholds; > Also unused Done Line 82: private final Map<String, SchedulingPolicy> schedulingPolicies; > The only policy is DEFAULT_POLICY so this is also not needed. Done Line 84: private final SchedulingPolicy defaultSchedulingPolicy; > Ditto Done 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 { > The only point of this class is capturing configuration exceptions and thro We still use the resource parsing code that throws this, e.g. in FairSchedulerConfiguration. I don't think we should change the API of those functions. 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 { > 103-165 duplicate code and functionality provided by impala.util.FileWatchS Yeah you're right, though I'm on the fence about how much we should doctor this right now given we know this works and that we don't have great automated end-to-end test coverage. Unless you feel strongly about cleaning up this now, I'd rather leave this as a cleanup opportunity for later (ramp up JIRA?) when we can add some additional tests which include modifying the allocation file. Line 223: Map<String, Long> minSharePreemptionTimeouts = new HashMap<>(); > Preemption configuration does nothing in Impala; could also be killed. I think it may be worth leaving this code as-is even if it's doing extra work. The AllocationConfiguration will ignore the things we don't care about, but leaving this as-is will make diff-ing this later much easier. 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); > This seems a pretty inefficient way to look up resources. This would be a c Agreed, though I don't think it's worth spending time improving this code. I'd rather not make diffing harder later. Do you feel strongly? 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; > This whole class is pretty useless and in Impala's case, only serves to thr Per my other comments, I'd prefer not to change the parsing code too much yet, in which case I think it'll just be worth keeping this dummy impl here for now. 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")); > Not clear to me why this is needed now. I'm actually not sure why the testResolvePrincipalName() was working before. RequestPoolService.resolveRequestPool() -> RPS.assignToPool() -> (impala) User.getShortName() -> (Hadoop) KerberosName.getShortName() And KerberosName.getShortName() relies on there being some rule set, otherwise a NPE is thrown. I think that there was some static initialization that just so happened to work before. That said, I think setting this is the right thing to do if we're going to resolve Kerberos principals. Let me know what you think. -- 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
