impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <>
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:


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
File common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/

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;

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;
File common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/

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.
File common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/

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.
File common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/

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'.
File common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/

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?
File fe/src/test/java/org/apache/impala/util/

Line 124:         new Configuration().get(HADOOP_SECURITY_AUTH_TO_LOCAL, "DEFAULT"));
Not clear to me why this is needed now.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zach Amsden <>
Gerrit-HasComments: Yes

View raw message