joseluisll commented on code in PR #8802:
URL: https://github.com/apache/hadoop/pull/8802#discussion_r4238402820
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractParentQueue.java:
##########
@@ -310,86 +264,46 @@ void setChildQueues(Collection<CSQueue> childQueues)
throws IOException {
|| parentCapacityType == QueueCapacityType.ABSOLUTE_RESOURCE) {
// We don't allow any mixed absolute + {weight, percentage} between
// children and parent
- if (childrenCapacityType != parentCapacityType &&
!this.getQueuePath()
- .equals(CapacitySchedulerConfiguration.ROOT)) {
- throw new IOException("Parent=" + this.getQueuePath()
- + ": When absolute minResource is used, we must make sure both
"
- + "parent and child all use absolute minResource");
+ String mixedError =
+ QueueCapacityChecks.checkAbsoluteResourceUsedByParentAndChildren(
+ getQueuePath(), parentCapacityType, childrenCapacityType);
+ if (mixedError != null) {
+ throw new IOException(mixedError);
}
// Ensure that for each parent queue: parent.min-resource >=
// Σ(child.min-resource).
for (String nodeLabel : queueCapacities.getExistingNodeLabels()) {
- Resource minRes = Resources.createResource(0, 0);
+ List<Resource> childrenMinResources = new
ArrayList<>(childQueues.size());
for (CSQueue queue : childQueues) {
- // Accumulate all min/max resource configured for all child
queues.
- Resources.addTo(minRes, queue.getQueueResourceQuotas()
+ childrenMinResources.add(queue.getQueueResourceQuotas()
.getConfiguredMinResource(nodeLabel));
}
Resource resourceByLabel =
labelManager.getResourceByLabel(nodeLabel,
queueContext.getClusterResource());
Resource parentMinResource =
usageTracker.getQueueResourceQuotas().getConfiguredMinResource(nodeLabel);
- if (!parentMinResource.equals(Resources.none()) &&
Resources.lessThan(
- resourceCalculator, resourceByLabel, parentMinResource,
minRes)) {
- throw new IOException(
- "Parent Queues" + " capacity: " + parentMinResource
- + " is less than" + " to its children:" + minRes
- + " for queue:" + getQueueName());
+ String minResourceError =
QueueCapacityChecks.checkChildrenMinResourceWithinParent(
+ getQueueName(), parentMinResource, childrenMinResources,
resourceByLabel,
+ resourceCalculator);
+ if (minResourceError != null) {
+ throw new IOException(minResourceError);
}
}
}
// When child uses percent
if (childrenCapacityType == QueueCapacityType.PERCENT) {
- float childrenPctSum = 0;
+ Collection<String> labels = queueCapacities.getExistingNodeLabels();
+ List<QueueCapacityChecks.QueueCapacityInput> children =
+ getCapacityInputs(childQueues, labels);
Review Comment:
nit: `getCapacityInputs(childQueues, labels)` builds the same inputs that
`getCapacityConfigurationTypeForQueues(childQueues)` built at line 259,
including a `checkConfigTypeIsAbsoluteResource` lookup per child and label that
the sum check doesn't need. Since that method is public and has other callers,
a private overload returning both the type and the inputs would let this branch
reuse them. Small cost, so up to you.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueCapacityChecks.java:
##########
@@ -0,0 +1,491 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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 java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.yarn.api.records.Resource;
+import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.AbstractCSQueue.CapacityConfigType;
+import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.AbstractParentQueue.QueueCapacityType;
+import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.QueueCapacityVector.ResourceUnitCapacityType;
+import org.apache.hadoop.yarn.util.resource.ResourceCalculator;
+import org.apache.hadoop.yarn.util.resource.Resources;
+
+/**
+ * Capacity checks run while a queue is built from configuration (legacy queue
+ * mode type and sum checks, and configured min/max resource checks).
+ * Every method returns null when the check passes, or the exact message of the
+ * exception the caller throws when it fails, so a validator working on
resolved
+ * configuration can share the same implementation as the queue constructors.
+ */
[email protected]
[email protected]
+public final class QueueCapacityChecks {
+ /**
+ * Tolerance used when comparing the sum of children capacities with 0 or 1.
+ */
+ public static final float CAPACITY_SUM_PRECISION = 0.0005f;
+
+ private QueueCapacityChecks() {}
+
+ /**
+ * The capacity configuration type of one label in legacy queue mode.
+ * @param absoluteResource whether the capacity of the label is configured
+ * as an absolute resource
+ * @return ABSOLUTE_RESOURCE or PERCENTAGE
+ */
+ public static CapacityConfigType capacityConfigTypeOf(
+ boolean absoluteResource) {
+ return absoluteResource ? CapacityConfigType.ABSOLUTE_RESOURCE
+ : CapacityConfigType.PERCENTAGE;
+ }
+
+ /**
+ * The capacity configuration type of one label outside legacy queue mode:
+ * ABSOLUTE_RESOURCE when every resource is absolute, otherwise PERCENTAGE
+ * (percentage, weight and mixed vectors).
+ * @param capacityVector the configured capacity vector of the label
+ * @return ABSOLUTE_RESOURCE or PERCENTAGE
+ */
+ public static CapacityConfigType capacityConfigTypeOf(
+ QueueCapacityVector capacityVector) {
+ Set<ResourceUnitCapacityType> definedCapacityTypes =
+ capacityVector.getDefinedCapacityTypes();
+ if (definedCapacityTypes.size() == 1 && definedCapacityTypes.iterator()
+ .next() == ResourceUnitCapacityType.ABSOLUTE) {
+ return CapacityConfigType.ABSOLUTE_RESOURCE;
+ }
+ return CapacityConfigType.PERCENTAGE;
+ }
+
+ /**
+ * The weight a capacity vector defines when it only has weights and every
+ * resource has the same one, for example 5w == [memory=5w, vcores=5w].
+ * Queue setup stores this weight in place of the configured weight.
+ * @param capacityVector the configured capacity vector of the label
+ * @return the common weight, or null if the vector does not define one
+ */
+ public static Float uniformWeightOf(QueueCapacityVector capacityVector) {
+ Set<ResourceUnitCapacityType> definedCapacityTypes =
+ capacityVector.getDefinedCapacityTypes();
+ if (definedCapacityTypes.size() != 1 || definedCapacityTypes.iterator()
+ .next() != ResourceUnitCapacityType.WEIGHT) {
+ return null;
+ }
+ Set<Double> weights = new HashSet<>();
+ for (String resourceName : capacityVector.getResourceNames()) {
+ weights.add(capacityVector.getResource(resourceName).getResourceValue());
+ }
+ return weights.size() == 1 ? weights.iterator().next().floatValue() : null;
+ }
+
+ /**
+ * Capacity related values of one queue for one node label, as seen by the
+ * legacy queue mode capacity type checks.
+ */
+ public static final class LabelCapacity {
+ /** Values reported by QueueCapacities for a label it does not know. */
+ public static final LabelCapacity UNSET = new LabelCapacity(0f, -1f,
false);
+
+ private final float capacity;
+ private final float weight;
+ private final boolean absolute;
+
+ /**
+ * @param capacity relative capacity in the range [0, 1]
+ * @param weight configured weight, -1 when no weight is configured
+ * @param absolute whether the capacity of the label is configured as an
+ * absolute resource
+ */
+ public LabelCapacity(float capacity, float weight, boolean absolute) {
+ this.capacity = capacity;
+ this.weight = weight;
+ this.absolute = absolute;
+ }
+
+ public float getCapacity() {
+ return capacity;
+ }
+
+ public float getWeight() {
+ return weight;
+ }
+
+ public boolean isAbsolute() {
+ return absolute;
+ }
+ }
+
+ /**
+ * Per label capacity values of one queue.
+ */
+ public static final class QueueCapacityInput {
+ private final String queuePath;
+ private final Map<String, LabelCapacity> byLabel;
+
+ public QueueCapacityInput(String queuePath,
+ Map<String, LabelCapacity> byLabel) {
+ this.queuePath = queuePath;
+ this.byLabel = Collections.unmodifiableMap(new HashMap<>(byLabel));
+ }
+
+ public String getQueuePath() {
+ return queuePath;
+ }
+
+ /**
+ * @param label node label
+ * @return the values of the label, or {@link LabelCapacity#UNSET}
+ */
+ public LabelCapacity get(String label) {
+ LabelCapacity labelCapacity = byLabel.get(label);
+ return labelCapacity == null ? LabelCapacity.UNSET : labelCapacity;
+ }
+ }
+
+ /**
+ * C05: the raw capacity of the root queue must be 100.
Review Comment:
The javadocs tag checks C05–C14 (and only in `QueueCapacityChecks`), but
neither the code, the JIRA nor the design doc on YARN-12010 defines those IDs,
and C01–C04 don't appear anywhere. If they're meant to become the `ruleId` from
the design doc, could they be defined in one place, or used consistently across
all the new check classes? Otherwise I'd drop them from this PR and add them
with the validation engine (YARN-12017).
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestQueueCapacityChecks.java:
##########
@@ -0,0 +1,321 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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 java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.hadoop.yarn.api.records.Resource;
+import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.AbstractCSQueue.CapacityConfigType;
+import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.AbstractParentQueue.QueueCapacityType;
+import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.QueueCapacityChecks.LabelCapacity;
+import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.QueueCapacityChecks.QueueCapacityInput;
+import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.QueueCapacityVector.ResourceUnitCapacityType;
+import org.apache.hadoop.yarn.util.resource.DefaultResourceCalculator;
+import org.apache.hadoop.yarn.util.resource.DominantResourceCalculator;
+import org.apache.hadoop.yarn.util.resource.ResourceCalculator;
+import org.apache.hadoop.yarn.util.resource.Resources;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+
+public class TestQueueCapacityChecks {
+ private static final ResourceCalculator DRC = new
DominantResourceCalculator();
+ private static final ResourceCalculator DEFAULT_RC = new
DefaultResourceCalculator();
+ private static final Resource CLUSTER = Resource.newInstance(100 * 1024,
100);
+ private static final Set<String> NO_LABEL = Collections.singleton("");
+
+ private static QueueCapacityInput queue(String path, float capacity, float
weight,
+ boolean absolute) {
+ Map<String, LabelCapacity> byLabel = new HashMap<>();
+ byLabel.put("", new LabelCapacity(capacity, weight, absolute));
+ return new QueueCapacityInput(path, byLabel);
+ }
+
+ private static QueueCapacityInput pct(String path, float capacity) {
+ return queue(path, capacity, -1f, false);
+ }
+
+ private static QueueCapacityInput weight(String path, float weight) {
+ return queue(path, 0f, weight, false);
+ }
+
+ private static QueueCapacityInput abs(String path) {
+ return queue(path, 0.3f, -1f, true);
+ }
+
+ @Test
+ public void testRootCapacity() {
+ assertNull(QueueCapacityChecks.checkRootCapacity("root", true, 100f));
+ assertNull(QueueCapacityChecks.checkRootCapacity("a", false, 50f));
+ assertEquals("Illegal capacity of 50.0 for queue root. Must be 100.0",
+ QueueCapacityChecks.checkRootCapacity("root", true, 50f));
+ }
+
+ @Test
+ public void testMaxResourceWithinParent() {
+ Resource parentMax = Resource.newInstance(10240, 10);
+ assertNull(QueueCapacityChecks.checkMaxResourceWithinParent("root.a",
+ Resource.newInstance(10240, 10), parentMax, CLUSTER, DRC));
+ // Unset parent max never fails
+ assertNull(QueueCapacityChecks.checkMaxResourceWithinParent("root.a",
+ Resource.newInstance(20480, 20), Resources.none(), CLUSTER, DRC));
+ assertEquals("Max resource configuration <memory:20480, vCores:20> is
greater than"
+ + " parents max value:<memory:10240, vCores:10> in queue:root.a",
+ QueueCapacityChecks.checkMaxResourceWithinParent("root.a",
+ Resource.newInstance(20480, 20), parentMax, CLUSTER, DRC));
+ // The default calculator only compares memory
+ assertNull(QueueCapacityChecks.checkMaxResourceWithinParent("root.a",
+ Resource.newInstance(10240, 20), parentMax, CLUSTER, DEFAULT_RC));
+ }
+
+ @Test
+ public void testInheritMaxResourceFromParent() {
+ Resource min = Resource.newInstance(1024, 1);
+ Resource max = Resource.newInstance(4096, 4);
+ Resource parentMax = Resource.newInstance(8192, 8);
+ assertSame(max, QueueCapacityChecks.inheritMaxResourceFromParent(min, max,
parentMax));
+ assertEquals(parentMax,
QueueCapacityChecks.inheritMaxResourceFromParent(min,
+ Resources.none(), parentMax));
+ assertEquals(Resources.none(),
QueueCapacityChecks.inheritMaxResourceFromParent(
+ Resources.none(), Resources.none(), parentMax));
+ assertEquals(Resources.none(),
QueueCapacityChecks.inheritMaxResourceFromParent(
+ min, Resources.none(), Resources.none()));
+ }
+
+ @Test
+ public void testMinResourceWithinMax() {
+ assertNull(QueueCapacityChecks.checkMinResourceWithinMax("root.a",
+ Resource.newInstance(1024, 1), Resource.newInstance(2048, 2), CLUSTER,
DRC));
+ assertNull(QueueCapacityChecks.checkMinResourceWithinMax("root.a",
+ Resource.newInstance(4096, 4), Resources.none(), CLUSTER, DRC));
+ assertEquals("Min resource configuration <memory:4096, vCores:4> is
greater than its"
+ + " max value:<memory:2048, vCores:2> in queue:root.a",
+ QueueCapacityChecks.checkMinResourceWithinMax("root.a",
+ Resource.newInstance(4096, 4), Resource.newInstance(2048, 2),
CLUSTER, DRC));
+ }
+
+ @Test
+ public void testCapacityConfigTypeConsistent() {
+ assertNull(QueueCapacityChecks.checkCapacityConfigTypeConsistent("root.a",
false, true,
+ CapacityConfigType.PERCENTAGE, CapacityConfigType.PERCENTAGE));
+ assertNull(QueueCapacityChecks.checkCapacityConfigTypeConsistent("root",
true, true,
+ CapacityConfigType.PERCENTAGE, CapacityConfigType.ABSOLUTE_RESOURCE));
+ assertNull(QueueCapacityChecks.checkCapacityConfigTypeConsistent("root.a",
false, false,
+ CapacityConfigType.PERCENTAGE, CapacityConfigType.ABSOLUTE_RESOURCE));
+ assertEquals("Queue 'root.a' should use either percentage based capacity
configuration"
+ + " or absolute resource.",
+ QueueCapacityChecks.checkCapacityConfigTypeConsistent("root.a", false,
true,
+ CapacityConfigType.PERCENTAGE,
CapacityConfigType.ABSOLUTE_RESOURCE));
+ }
+
+ @Test
+ public void testCapacityTypesNotMixed() {
+ assertNull(QueueCapacityChecks.checkCapacityTypesNotMixed("root", NO_LABEL,
+ Arrays.asList(pct("root.a", 0.4f), pct("root.b", 0.6f))));
+ assertNull(QueueCapacityChecks.checkCapacityTypesNotMixed("root", NO_LABEL,
+ Collections.emptyList()));
+ // 0% does not count as percentage
+ assertNull(QueueCapacityChecks.checkCapacityTypesNotMixed("root", NO_LABEL,
+ Arrays.asList(pct("root.a", 0f), weight("root.b", 1f))));
+ // An absolute queue clears the percentage flag of the queues before it
+ assertNull(QueueCapacityChecks.checkCapacityTypesNotMixed("root", NO_LABEL,
+ Arrays.asList(pct("root.a", 0.5f), abs("root.b"))));
+ assertEquals("Parent queue 'root' have children queue used mixed of
weight mode,"
+ + " percentage and absolute mode, it is not allowed, please double
check,"
+ + " details:{Queue=root.a, label= uses absolute mode}. "
+ + "{Queue=root.b, label= uses percentage mode}. ",
+ QueueCapacityChecks.checkCapacityTypesNotMixed("root", NO_LABEL,
+ Arrays.asList(abs("root.a"), pct("root.b", 0.5f))));
+ assertEquals("Parent queue 'root' have children queue used mixed of
weight mode,"
+ + " percentage and absolute mode, it is not allowed, please double
check,"
+ + " details:{Queue=root.a, label= uses percentage mode}. "
+ + "{Queue=root.b, label= uses weight mode}. "
+ + "{Queue=root.b, label= uses percentage mode}. ",
+ QueueCapacityChecks.checkCapacityTypesNotMixed("root", NO_LABEL,
+ Arrays.asList(pct("root.a", 0.5f), weight("root.b", 1f))));
+ // Skipped when the first queue is root
+ assertNull(QueueCapacityChecks.checkCapacityTypesNotMixed("root", NO_LABEL,
+ Arrays.asList(pct("root", 1f), weight("root.b", 1f))));
+ }
+
+ @Test
+ public void testCapacityTypesNotMixedOnlyChecksGivenLabels() {
+ Map<String, LabelCapacity> byLabel = new HashMap<>();
+ byLabel.put("", new LabelCapacity(0.5f, -1f, false));
+ byLabel.put("x", new LabelCapacity(0f, 1f, false));
+ QueueCapacityInput mixed = new QueueCapacityInput("root.a", byLabel);
+ assertNull(QueueCapacityChecks.checkCapacityTypesNotMixed("root", NO_LABEL,
+ Collections.singletonList(mixed)));
+ Set<String> labels = new LinkedHashSet<>(Arrays.asList("", "x"));
+ assertEquals("Parent queue 'root.a' have children queue used mixed of
weight mode,"
+ + " percentage and absolute mode, it is not allowed, please double
check,"
+ + " details:{Queue=root.a, label= uses percentage mode}. "
+ + "{Queue=root.a, label=x uses weight mode}. "
+ + "{Queue=root.a, label=x uses percentage mode}. ",
+ QueueCapacityChecks.checkCapacityTypesNotMixed("root.a", labels,
+ Collections.singletonList(mixed)));
+ // A label missing from the input reads as unset
+ assertEquals(LabelCapacity.UNSET, mixed.get("y"));
+ }
+
+ @Test
+ public void testCapacityConfigurationType() {
+ assertEquals(QueueCapacityType.WEIGHT,
QueueCapacityChecks.getCapacityConfigurationType(
+ NO_LABEL, Collections.emptyList()));
+ assertEquals(QueueCapacityType.WEIGHT,
QueueCapacityChecks.getCapacityConfigurationType(
+ NO_LABEL, Arrays.asList(pct("root.a", 0f), weight("root.b", 1f))));
+ assertEquals(QueueCapacityType.ABSOLUTE_RESOURCE,
+ QueueCapacityChecks.getCapacityConfigurationType(NO_LABEL,
+ Arrays.asList(pct("root.a", 0.5f), abs("root.b"))));
+ assertEquals(QueueCapacityType.PERCENT,
QueueCapacityChecks.getCapacityConfigurationType(
+ NO_LABEL, Arrays.asList(pct("root.a", 0.5f), pct("root.b", 0.5f))));
+ }
+
+ @Test
+ public void testAbsoluteResourceUsedByParentAndChildren() {
+
assertNull(QueueCapacityChecks.checkAbsoluteResourceUsedByParentAndChildren("root.a",
+ QueueCapacityType.ABSOLUTE_RESOURCE,
QueueCapacityType.ABSOLUTE_RESOURCE));
+
assertNull(QueueCapacityChecks.checkAbsoluteResourceUsedByParentAndChildren("root.a",
+ QueueCapacityType.PERCENT, QueueCapacityType.WEIGHT));
+
assertNull(QueueCapacityChecks.checkAbsoluteResourceUsedByParentAndChildren("root",
+ QueueCapacityType.PERCENT, QueueCapacityType.ABSOLUTE_RESOURCE));
+ String expected = "Parent=root.a: When absolute minResource is used, we
must make sure"
+ + " both parent and child all use absolute minResource";
+ assertEquals(expected,
QueueCapacityChecks.checkAbsoluteResourceUsedByParentAndChildren(
+ "root.a", QueueCapacityType.PERCENT,
QueueCapacityType.ABSOLUTE_RESOURCE));
+ assertEquals(expected,
QueueCapacityChecks.checkAbsoluteResourceUsedByParentAndChildren(
+ "root.a", QueueCapacityType.ABSOLUTE_RESOURCE,
QueueCapacityType.WEIGHT));
+ }
+
+ @Test
+ public void testChildrenMinResourceWithinParent() {
+ List<Resource> children = Arrays.asList(Resource.newInstance(2048, 2),
+ Resource.newInstance(3072, 3));
+ assertNull(QueueCapacityChecks.checkChildrenMinResourceWithinParent("a",
+ Resource.newInstance(5120, 5), children, CLUSTER, DRC));
+ assertNull(QueueCapacityChecks.checkChildrenMinResourceWithinParent("a",
+ Resources.none(), children, CLUSTER, DRC));
+ assertEquals("Parent Queues capacity: <memory:4096, vCores:4> is less than
to its"
+ + " children:<memory:5120, vCores:5> for queue:a",
+ QueueCapacityChecks.checkChildrenMinResourceWithinParent("a",
+ Resource.newInstance(4096, 4), children, CLUSTER, DRC));
+ }
+
+ @Test
+ public void testChildrenCapacitySum() {
+ List<QueueCapacityInput> full = Arrays.asList(pct("root.a.x", 0.4f),
+ pct("root.a.y", 0.6f));
+ List<QueueCapacityInput> zero = Arrays.asList(pct("root.a.x", 0f),
+ pct("root.a.y", 0f));
+ List<QueueCapacityInput> partial = Arrays.asList(pct("root.a.x", 0.4f),
+ pct("root.a.y", 0.5f));
+
+ assertNull(QueueCapacityChecks.checkChildrenCapacitySum("a", "",
+ QueueCapacityType.PERCENT, 0.5f, false, full));
+ // Within precision
+ assertNull(QueueCapacityChecks.checkChildrenCapacitySum("a", "",
+ QueueCapacityType.PERCENT, 0.5f, false,
+ Arrays.asList(pct("root.a.x", 0.4f), pct("root.a.y", 0.5996f))));
+
+ // C12
+ assertEquals("Illegal capacity sum of 0.9 for children of queue a for
label=."
+ + " It should be either 0 or 1.0",
+ QueueCapacityChecks.checkChildrenCapacitySum("a", "",
+ QueueCapacityType.PERCENT, 0.5f, true, partial));
+
+ // C13
+ assertEquals("Illegal capacity sum of 0.0 for children of queue a for
label=x."
+ + " It is set to 0, but parent percent != 0, and doesn't allow
children"
+ + " capacity to set to 0",
+ QueueCapacityChecks.checkChildrenCapacitySum("a", "x",
+ QueueCapacityType.PERCENT, 0.5f, false, Arrays.asList(
+ labeled("root.a.x", "x", 0f), labeled("root.a.y", "x", 0f))));
+ assertNull(QueueCapacityChecks.checkChildrenCapacitySum("a", "",
+ QueueCapacityType.PERCENT, 0.5f, true, zero));
+ assertNull(QueueCapacityChecks.checkChildrenCapacitySum("a", "",
+ QueueCapacityType.PERCENT, 0f, false, zero));
+ assertNull(QueueCapacityChecks.checkChildrenCapacitySum("a", "",
+ QueueCapacityType.PERCENT, 0.0004f, false, zero));
+ assertNull(QueueCapacityChecks.checkChildrenCapacitySum("a", "",
+ QueueCapacityType.WEIGHT, 0.5f, false, zero));
+
+ // C14
+ assertEquals("Illegal capacity sum of 1.0 for children of queue a for
label=."
+ + " queue=a has zero capacity, but childqueues have positive
capacities",
Review Comment:
Not for this PR: the new tests pin two existing message typos ("childqueues"
here, "theconfiguration" in `TestQueueStructureChecks`). Fine to keep them
identical here, but they could be fixed in a follow-up together with these
assertions.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueHierarchyTransitionChecks.java:
##########
@@ -0,0 +1,212 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.yarn.api.records.QueueState;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Checks of the transition from the existing queue hierarchy to a newly
configured one:
+ * queue removal, moves and conversions between leaf, parent and auto create
enabled parent
+ * queues. Every check compares an existing queue with its counterpart in the
new hierarchy and
+ * returns null when it passes, or the error message when it fails; the caller
decides which
+ * exception to throw.
+ */
[email protected]
[email protected]
+public final class QueueHierarchyTransitionChecks {
+ private static final Logger LOG = LoggerFactory.getLogger(
+ CapacitySchedulerConfigValidator.class);
+
+ private QueueHierarchyTransitionChecks() {
+ }
+
+ /**
+ * The kind of a queue, as far as the hierarchy transition checks are
concerned.
+ */
+ public enum QueueKind {
Review Comment:
`QueueHierarchyTransitionChecks.QueueKind` and
`PlacementRuleChecks.QueueKind` share a name, but `PARENT` means different
things: the first includes `PlanQueue` (any non-managed `AbstractParentQueue`),
the second means only `ParentQueue` (`PlanQueue` is `OTHER_PARENT`). Each also
has its own `instanceof` classifier
(`CapacitySchedulerConfigValidator#toSnapshot` and `QueueRef#of`). Since
YARN-12016 will build these from resolved config, could there be one enum
(LEAF, PARENT, MANAGED_PARENT, OTHER_PARENT, OTHER) and one classifier, with
the transition checks treating OTHER_PARENT as a parent?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueHierarchyTransitionChecks.java:
##########
@@ -0,0 +1,212 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.yarn.api.records.QueueState;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Checks of the transition from the existing queue hierarchy to a newly
configured one:
+ * queue removal, moves and conversions between leaf, parent and auto create
enabled parent
+ * queues. Every check compares an existing queue with its counterpart in the
new hierarchy and
+ * returns null when it passes, or the error message when it fails; the caller
decides which
+ * exception to throw.
+ */
[email protected]
[email protected]
+public final class QueueHierarchyTransitionChecks {
+ private static final Logger LOG = LoggerFactory.getLogger(
Review Comment:
nit: I assume using `CapacitySchedulerConfigValidator.class` for the logger
is deliberate, to keep the log category the "Not a valid queue state" warning
had before. A short comment would save the next reader from "fixing" it.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueStateHelper.java:
##########
@@ -78,21 +78,111 @@ private static void reinitializeState(
private static void initializeState(
AbstractCSQueue queue, QueueState configuredState, QueueState
parentState) {
- QueueState currentState = configuredState == null ? DEFAULT_STATE :
configuredState;
-
if (parentState != null) {
- if (configuredState == QueueState.RUNNING && parentState !=
QueueState.RUNNING) {
- throw new IllegalArgumentException(
- "The parent queue:" + queue.getParent().getQueuePath()
- + " cannot be STOPPED as the child queue:" +
queue.getQueuePath()
- + " is in RUNNING state.");
+ String initialStateError = checkInitialState(new InitialStateInput(
+ queue.getQueuePath(), configuredState,
queue.getParent().getQueuePath(), parentState));
+ if (initialStateError != null) {
+ throw new IllegalArgumentException(initialStateError);
}
+ }
- if (configuredState == null) {
- currentState = parentState == QueueState.DRAINING ? QueueState.STOPPED
: parentState;
- }
+ queue.updateQueueState(getInitialState(configuredState, parentState));
+ }
+
+ /**
+ * Checks that the configured state is one that can be set in the
configuration.
+ * The parsing of the value itself happens in
+ * {@link CapacitySchedulerConfiguration#getConfiguredState(QueuePath)}.
+ * @param configuredState the configured state of the queue, null if not
configured
+ * @return null if the state is valid, otherwise the error message
+ */
+ public static String checkConfiguredState(QueueState configuredState) {
+ if (configuredState != null &&
!VALID_STATE_CONFIGURATIONS.contains(configuredState)) {
+ return "Invalid queue state configuration. We can only use RUNNING or
STOPPED.";
+ }
+ return null;
+ }
+
+ /**
+ * Checks that a newly created queue configured as RUNNING is not placed
under a parent that
+ * is not RUNNING.
+ * @param input the queue and its parent state
+ * @return null if the check passes, otherwise the error message
+ */
+ public static String checkInitialState(InitialStateInput input) {
+ if (input.getParentState() != null && input.getConfiguredState() ==
QueueState.RUNNING
+ && input.getParentState() != QueueState.RUNNING) {
+ return "The parent queue:" + input.getParentQueuePath()
+ + " cannot be STOPPED as the child queue:" + input.getQueuePath()
+ + " is in RUNNING state.";
+ }
+ return null;
+ }
+
+ /**
+ * Checks that a queue is activated only under a RUNNING parent. The
+ * exception type of a failure is {@link YarnException}.
+ * @param parentQueuePath the full path of the parent, null for root
+ * @param parentState the state of the parent, null for root
+ * @return null if the queue can be activated, otherwise the error message
+ */
+ public static String checkParentRunning(String parentQueuePath,
Review Comment:
nit: `checkParentRunning` uses `parentQueuePath == null` to mean "no
parent". The only caller already handles a null parent, so it could take just
the parent's state and leave null handling to the caller.
--
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]
