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 Mon, 18 Sep 2017 23:24:51 GMT
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 <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