Zach Amsden has posted comments on this change. Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar ......................................................................
Patch Set 4: (17 comments) Looks good but I have a few comments - there is a lot more that could be pruned. Let me know which if any you might pursue and I can give a +1 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 kill this. Line 64: private final Map<String, Map<QueueACL, AccessControlList>> queueAcls; Useless (just SUBMIT_APPLICATIONS) Line 69: private final Map<String, Long> minSharePreemptionTimeouts; Unused by Impala Line 74: private final Map<String, Long> fairSharePreemptionTimeouts; Also unused by Impala Line 80: private final Map<String, Float> fairSharePreemptionThresholds; Also unused Line 82: private final Map<String, SchedulingPolicy> schedulingPolicies; The only policy is DEFAULT_POLICY so this is also not needed. Line 84: private final SchedulingPolicy defaultSchedulingPolicy; Ditto 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 throwing, but if you remove most of the throws, maybe this class is not necessary either. 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.FileWatchService (minus the ALLOC_RELOAD_WAIT_MS check); we only needed this code before because this is the way yarn worked internally, but it would be better to have just one implementation here - maybe switch this to use FileWatchService. Line 171: * classpath, but loaded like a regular File. This seems like unnecessary functionality that we could deprecate if desired. Line 223: Map<String, Long> minSharePreemptionTimeouts = new HashMap<>(); Preemption configuration does nothing in Impala; could also be killed. Line 226: Map<String, Map<QueueACL, AccessControlList>> queueAcls = new HashMap<>(); Impala only uses a single queueACL, SUBMIT_APPLICATIONS. We could pull the entire QueueACL class into impala to get rid of one more Yarnish dependency. Line 323: } else if ("defaultQueueSchedulingPolicy".equals(element.getTagName()) Here's the useless code and exception throwing. Line 533: public interface Listener { This can probably go away too. 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 candidate for memoizing if this is called often with the same 'units'. 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 throw exceptions when the SchedulingPolicy can't be parsed as a valid policy. Since that is now impossible, maybe just remove the class? 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. -- 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
