shuzirra commented on a change in pull request #3470: URL: https://github.com/apache/hadoop/pull/3470#discussion_r808597004
########## File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CalculationContext.java ########## @@ -0,0 +1,70 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity; + +import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.QueueCapacityVector.QueueCapacityVectorEntry; + +/** + * A storage class that wraps arguments used in a resource calculation iteration. + */ +public class CalculationContext { + private final String resourceName; + private final QueueCapacityVector.ResourceUnitCapacityType capacityType; + private final CSQueue childQueue; Review comment: This might be nitpicking, but it was a bit confusing me. Why is it called child queue? I understand that we create a CalculationContext object for every child queue, however from the CalculationContext's point of view, it's not a child, but the actual queue we are doing the calculation. Calling this a child queue suggests that there is a queue we are working with and it is it's children, however this class only works with the child queue, and there is no "actual queue". So I think it would be better to call it simply queue. To my mind this is the desired terminology: ParentQueue (parent of the queue we are working with) | Queue (the queue we are working with) | ChildQueue (one of our work queue's children) ########## File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractQueueCapacityCalculator.java ########## @@ -84,12 +76,6 @@ public abstract float calculateMaximumResource(ResourceCalculationDriver resourc * calculation should be made */ public void calculateResourcePrerequisites(ResourceCalculationDriver resourceCalculationDriver) { Review comment: This method is pretty empty, we should either remove it, or make it abstract. ########## File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ResourceCalculationDriver.java ########## @@ -20,64 +20,63 @@ import org.apache.commons.lang3.tuple.ImmutablePair; import org.apache.commons.lang3.tuple.Pair; -import org.apache.hadoop.thirdparty.com.google.common.collect.ImmutableSet; import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.QueueCapacityVector.QueueCapacityVectorEntry; import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.QueueCapacityVector.ResourceUnitCapacityType; +import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.QueueUpdateWarning.QueueUpdateWarningType; +import org.apache.hadoop.yarn.util.UnitsConversionUtil; import java.util.Collection; import java.util.HashMap; import java.util.Map; -import java.util.Set; import static org.apache.hadoop.yarn.api.records.ResourceInformation.MEMORY_URI; /** - * Drives the main logic of resource calculation for all children under a parent queue. Acts as a - * bookkeeper of disposable update information that is used by all children under a common parent. + * Drives the main logic of resource calculation for all children under a queue. Acts as a + * bookkeeper of disposable update information that is used by all children under the common parent. */ public class ResourceCalculationDriver { - protected static final Set<ResourceUnitCapacityType> CALCULATOR_PRECEDENCE = - ImmutableSet.of( + protected static final ResourceUnitCapacityType[] CALCULATOR_PRECEDENCE = + new ResourceUnitCapacityType[] { ResourceUnitCapacityType.ABSOLUTE, ResourceUnitCapacityType.PERCENTAGE, - ResourceUnitCapacityType.WEIGHT); + ResourceUnitCapacityType.WEIGHT}; + static final String MB_UNIT = "Mi"; protected final QueueResourceRoundingStrategy roundingStrategy = new DefaultQueueResourceRoundingStrategy(CALCULATOR_PRECEDENCE); - protected final CSQueue parent; + protected final CSQueue queue; protected final QueueCapacityUpdateContext updateContext; protected final Map<ResourceUnitCapacityType, AbstractQueueCapacityCalculator> calculators; protected final Collection<String> definedResources; - protected final Map<String, ResourceVector> overallRemainingResource = new HashMap<>(); - protected final Map<String, ResourceVector> batchRemainingResource = new HashMap<>(); + protected final Map<String, ResourceVector> overallRemainingResourcePerLabel = new HashMap<>(); + protected final Map<String, ResourceVector> batchRemainingResourcePerLabel = new HashMap<>(); // Used by ABSOLUTE capacity types - protected final Map<String, ResourceVector> normalizedResourceRatio = new HashMap<>(); - // Used by WEIGHT capacity typet js - protected final Map<String, Map<String, Float>> sumWeightsPerLabel = new HashMap<>(); - - protected String currentResourceName; - protected AbstractQueueCapacityCalculator currentCalculator; - protected CSQueue currentChild; - protected Map<String, Float> usedResourceByCurrentCalculator = new HashMap<>(); + protected final Map<String, ResourceVector> normalizedResourceRatioPerLabel = new HashMap<>(); + // Used by WEIGHT capacity types + protected final Map<String, Map<String, Double>> sumWeightsPerLabel = new HashMap<>(); + protected Map<String, Double> usedResourceByCurrentCalculatorPerLabel = new HashMap<>(); public ResourceCalculationDriver( - CSQueue parent, QueueCapacityUpdateContext updateContext, + CSQueue queue, QueueCapacityUpdateContext updateContext, Map<ResourceUnitCapacityType, AbstractQueueCapacityCalculator> calculators, Collection<String> definedResources) { - this.parent = parent; + this.queue = queue; this.updateContext = updateContext; this.calculators = calculators; this.definedResources = definedResources; + setNormalizedResourceRatio(); } + /** * Returns the parent that is driving the calculation. * * @return a common parent queue */ - public CSQueue getParent() { - return parent; + public CSQueue getQueue() { + return queue; } Review comment: You might consider adding a helper getChildren method as well, you have a lot of resourceCalculator.getQueue().getChildren() calls, you could proxy the queue.getChilder() as resourceCalculator.getChildren(). It's just a suggestion, it's fine this way as well. ########## File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueCapacityHandler.java ########## @@ -70,61 +70,133 @@ public CapacitySchedulerQueueCapacityHandler(RMNodeLabelsManager labelsManager) } /** - * Updates the resource and metrics values for a queue, its siblings and descendants. + * Updates the resource and metrics values of all children under a specific queue. * These values are calculated at runtime. * * @param clusterResource resource of the cluster - * @param queue queue to update + * @param queue parent queue whose children will be updated * @return update context that contains information about the update phase */ - public QueueCapacityUpdateContext update(Resource clusterResource, CSQueue queue) { + public QueueCapacityUpdateContext updateChildren(Resource clusterResource, CSQueue queue) { ResourceLimits resourceLimits = new ResourceLimits(clusterResource); QueueCapacityUpdateContext updateContext = new QueueCapacityUpdateContext(clusterResource, labelsManager); - if (queue.getQueuePath().equals(ROOT)) { - updateRoot(queue, updateContext, resourceLimits); - updateChildren(queue, updateContext, resourceLimits); - } else { - updateChildren(queue.getParent(), updateContext, resourceLimits); - } - + update(queue, updateContext, resourceLimits); return updateContext; } - private void updateRoot( - CSQueue queue, QueueCapacityUpdateContext updateContext, ResourceLimits resourceLimits) { - RootCalculationDriver rootCalculationDriver = new RootCalculationDriver(queue, updateContext, + /** + * Updates the resource and metrics value of the root queue. Root queue always has percentage + * capacity type and is assigned the cluster resource as its minimum and maximum effective + * resource. + * @param rootQueue root queue + * @param clusterResource cluster resource + */ + public void updateRoot(CSQueue rootQueue, Resource clusterResource) { + ResourceLimits resourceLimits = new ResourceLimits(clusterResource); + QueueCapacityUpdateContext updateContext = + new QueueCapacityUpdateContext(clusterResource, labelsManager); + + RootCalculationDriver rootCalculationDriver = new RootCalculationDriver(rootQueue, updateContext, rootCalculator, definedResources); rootCalculationDriver.calculateResources(); - queue.refreshAfterResourceCalculation(updateContext.getUpdatedClusterResource(), resourceLimits); + rootQueue.refreshAfterResourceCalculation(updateContext.getUpdatedClusterResource(), resourceLimits); } - private void updateChildren( - CSQueue parent, QueueCapacityUpdateContext updateContext, - ResourceLimits resourceLimits) { - if (parent == null || CollectionUtils.isEmpty(parent.getChildQueues())) { + private void update( + CSQueue queue, QueueCapacityUpdateContext updateContext, ResourceLimits resourceLimits) { + if (queue == null || CollectionUtils.isEmpty(queue.getChildQueues())) { return; } ResourceCalculationDriver resourceCalculationDriver = new ResourceCalculationDriver( - parent, updateContext, calculators, definedResources); + queue, updateContext, calculators, definedResources); resourceCalculationDriver.calculateResources(); updateChildrenAfterCalculation(resourceCalculationDriver, resourceLimits); } private void updateChildrenAfterCalculation( ResourceCalculationDriver resourceCalculationDriver, ResourceLimits resourceLimits) { - for (CSQueue childQueue : resourceCalculationDriver.getParent().getChildQueues()) { - resourceCalculationDriver.setCurrentChild(childQueue); - resourceCalculationDriver.updateChildCapacities(); - ResourceLimits childLimit = ((ParentQueue) resourceCalculationDriver.getParent()).getResourceLimitsOfChild( - childQueue, resourceCalculationDriver.getUpdateContext().getUpdatedClusterResource(), resourceLimits, NO_LABEL, false); + ParentQueue parentQueue = (ParentQueue) resourceCalculationDriver.getQueue(); + for (CSQueue childQueue : parentQueue.getChildQueues()) { + updateChildCapacities(resourceCalculationDriver, childQueue); + + ResourceLimits childLimit = parentQueue.getResourceLimitsOfChild(childQueue, + resourceCalculationDriver.getUpdateContext().getUpdatedClusterResource(), + resourceLimits, NO_LABEL, false); childQueue.refreshAfterResourceCalculation(resourceCalculationDriver.getUpdateContext() .getUpdatedClusterResource(), childLimit); - updateChildren(childQueue, resourceCalculationDriver.getUpdateContext(), childLimit); + + update(childQueue, resourceCalculationDriver.getUpdateContext(), childLimit); + } + } + + /** + * Updates the capacity values of the currently evaluated child. + * @param childQueue child queue on which the capacities are set + */ + private void updateChildCapacities(ResourceCalculationDriver resourceCalculationDriver, CSQueue childQueue) { Review comment: This is a bit misleading again, this is just simply updateQueueCapacities, there is nothing child specific thing involved here. So we can simply go with the queue terminology. Child should be always relative to something, but if the only thing we are working with is the child, then it's not a child but a queue. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
