szilard-nemeth commented on code in PR #5332:
URL: https://github.com/apache/hadoop/pull/5332#discussion_r1181971717
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServicesCapacitySchedDynamicConfig.java:
##########
@@ -145,8 +146,9 @@ public void
testSchedulerResponseWeightModeWithAutoCreatedQueues()
throws Exception {
Configuration config = CSConfigGenerator
.createWeightConfigWithAutoQueueCreationEnabled();
- config.setInt(CapacitySchedulerConfiguration
- .getQueuePrefix("root.autoParent1") +
+ QueuePath autoPArent1 = new QueuePath("root.autoParent1");
Review Comment:
Nit: Typo in name, should be autoParent1
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1560,82 +1545,82 @@ void setWorkflowPriorityMappings(
public boolean isReservable(String queue) {
boolean isReservable =
- getBoolean(getQueuePrefix(queue) + IS_RESERVABLE, false);
+ getBoolean(getQueuePrefix(new QueuePath(queue)) + IS_RESERVABLE,
false);
return isReservable;
}
public void setReservable(String queue, boolean isReservable) {
- setBoolean(getQueuePrefix(queue) + IS_RESERVABLE, isReservable);
+ setBoolean(getQueuePrefix(new QueuePath(queue)) + IS_RESERVABLE,
isReservable);
LOG.debug("here setReservableQueue: queuePrefix={}, isReservableQueue={}",
- getQueuePrefix(queue), isReservable(queue));
+ getQueuePrefix(new QueuePath(queue)), isReservable(queue));
}
@Override
public long getReservationWindow(String queue) {
long reservationWindow =
- getLong(getQueuePrefix(queue) + RESERVATION_WINDOW,
+ getLong(getQueuePrefix(new QueuePath(queue)) + RESERVATION_WINDOW,
DEFAULT_RESERVATION_WINDOW);
return reservationWindow;
}
@Override
public float getAverageCapacity(String queue) {
float avgCapacity =
- getFloat(getQueuePrefix(queue) + AVERAGE_CAPACITY,
+ getFloat(getQueuePrefix(new QueuePath(queue)) + AVERAGE_CAPACITY,
MAXIMUM_CAPACITY_VALUE);
return avgCapacity;
}
@Override
public float getInstantaneousMaxCapacity(String queue) {
float instMaxCapacity =
- getFloat(getQueuePrefix(queue) + INSTANTANEOUS_MAX_CAPACITY,
+ getFloat(getQueuePrefix(new QueuePath(queue)) +
INSTANTANEOUS_MAX_CAPACITY,
MAXIMUM_CAPACITY_VALUE);
return instMaxCapacity;
}
- public void setInstantaneousMaxCapacity(String queue, float instMaxCapacity)
{
+ public void setInstantaneousMaxCapacity(QueuePath queue, float
instMaxCapacity) {
setFloat(getQueuePrefix(queue) + INSTANTANEOUS_MAX_CAPACITY,
instMaxCapacity);
}
- public void setReservationWindow(String queue, long reservationWindow) {
+ public void setReservationWindow(QueuePath queue, long reservationWindow) {
setLong(getQueuePrefix(queue) + RESERVATION_WINDOW, reservationWindow);
}
- public void setAverageCapacity(String queue, float avgCapacity) {
+ public void setAverageCapacity(QueuePath queue, float avgCapacity) {
setFloat(getQueuePrefix(queue) + AVERAGE_CAPACITY, avgCapacity);
}
@Override
public String getReservationAdmissionPolicy(String queue) {
Review Comment:
getReservationAdmissionPolicy / setReservationAdmissionPolicy has different
API. The getter works with queue name (path?) but the setter works with
QueuePath object. I think this should be streamlined.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueuePrefixes.java:
##########
@@ -0,0 +1,58 @@
+/**
+ * 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.yarn.nodelabels.CommonNodeLabelsManager;
+
+import static
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.ACCESSIBLE_NODE_LABELS;
+import static
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.AUTO_CREATED_LEAF_QUEUE_TEMPLATE_PREFIX;
+import static
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.DOT;
+import static
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.PREFIX;
+
+public final class QueuePrefixes {
+
+ private QueuePrefixes() {
+ }
+
+ public static String getQueuePrefix(QueuePath queuePath) {
+ String queueName = PREFIX + queuePath.getFullPath() + DOT;
Review Comment:
Nit: No need for local, can be returned inline.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/test/java/org/apache/hadoop/yarn/server/router/webapp/TestableFederationInterceptorREST.java:
##########
@@ -82,11 +83,11 @@ protected void setupResourceManager() throws IOException {
CapacitySchedulerConfiguration conf = new
CapacitySchedulerConfiguration();
// Define default queue
- conf.setCapacity(QUEUE_DEFAULT_FULL, 20);
+ conf.setCapacity(DEFAULT_QUEUE_PATH, 20);
// Define dedicated queues
String[] queues = new String[]{QUEUE_DEFAULT, QUEUE_DEDICATED};
- conf.setQueues(CapacitySchedulerConfiguration.ROOT, queues);
- conf.setCapacity(QUEUE_DEDICATED_FULL, 80);
+ conf.setQueues(ROOT_QUEUE_PATH, queues);
+ conf.setCapacity(DEFAULT_QUEUE_PATH, 80);
Review Comment:
Shouldn't this be the dedicated queue?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/QueuePlacementConverter.java:
##########
@@ -187,7 +188,7 @@ private Rule createNestedRule(Policy policy,
boolean create,
FSConfigToCSConfigRuleHandler ruleHandler,
boolean fsParentCreate,
- String parentQueue,
+ QueuePath parentQueue,
Review Comment:
Was this really the full queue path before? or just the queue name?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueuePath.java:
##########
@@ -105,6 +108,14 @@ public boolean hasEmptyPart() {
return false;
}
Review Comment:
Have you added UTs for all new functionality of this class?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/ConfigurationUpdateAssembler.java:
##########
@@ -76,19 +76,20 @@ private static void removeQueue(
throw new IOException("Queue " + queueToRemove + " not found");
}
siblingQueues.remove(queueName);
- String parentQueuePath = queueToRemove.substring(0, queueToRemove
+ String parentPath = queueToRemove.substring(0, queueToRemove
.lastIndexOf('.'));
+ QueuePath parentQueuePath = new QueuePath(parentPath);
proposedConf.setQueues(parentQueuePath, siblingQueues.toArray(
new String[0]));
String queuesConfig = CapacitySchedulerConfiguration.PREFIX
- + parentQueuePath + CapacitySchedulerConfiguration.DOT
+ + parentPath + CapacitySchedulerConfiguration.DOT
+ CapacitySchedulerConfiguration.QUEUES;
if (siblingQueues.isEmpty()) {
confUpdate.put(queuesConfig, null);
// Unset Ordering Policy of Leaf Queue converted from
// Parent Queue after removeQueue
String queueOrderingPolicy = CapacitySchedulerConfiguration.PREFIX
- + parentQueuePath + CapacitySchedulerConfiguration.DOT
+ + parentPath + CapacitySchedulerConfiguration.DOT
+ ORDERING_POLICY;
proposedConf.unset(queueOrderingPolicy);
confUpdate.put(queueOrderingPolicy, null);
Review Comment:
Can we move all queue string operations from addQueue / removeQueue methods
to the QueuePath object?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AutoCreatedQueueTemplate.java:
##########
@@ -154,46 +148,26 @@ public void
setTemplateEntriesForChild(CapacitySchedulerConfiguration conf,
*/
private void setTemplateConfigEntries(CapacitySchedulerConfiguration
configuration,
QueuePath queuePath) {
- ConfigurationProperties configurationProperties =
- configuration.getConfigurationProperties();
-
- List<String> queuePathParts = Lists.newArrayList(queuePath.iterator());
-
- if (queuePathParts.size() <= 1 && !queuePath.isRoot()) {
- // This is an invalid queue path
- return;
- }
- int queuePathMaxIndex = queuePathParts.size() - 1;
-
- // start with the most explicit format (without wildcard)
- int wildcardLevel = 0;
- // root can not be wildcarded
- int supportedWildcardLevel = Math.min(queuePathMaxIndex,
-
configuration.getMaximumAutoCreatedQueueDepth(queuePath.getFullPath()));
- // Allow root to have template properties
- if (queuePath.isRoot()) {
- supportedWildcardLevel = 0;
- }
-
- // Collect all template entries
- while (wildcardLevel <= supportedWildcardLevel) {
- String templateQueuePath = String.join(".", queuePathParts);
- // Get all configuration entries with
- // yarn.scheduler.capacity.<queuePath> prefix
- Map<String, String> queueProps = configurationProperties
- .getPropertiesWithPrefix(getQueuePrefix(templateQueuePath));
-
- // Store template, parent-template and leaf-template properties
- for (Map.Entry<String, String> entry : queueProps.entrySet()) {
- storeConfiguredTemplates(entry.getKey(), entry.getValue());
+ if (!queuePath.isInvalid()) {
+ ConfigurationProperties configurationProperties =
+ configuration.getConfigurationProperties();
+
+ int maxAutoCreatedQueueDepth = configuration
+ .getMaximumAutoCreatedQueueDepth(queuePath);
+ List<QueuePath> wildcardedQueuePaths =
+ queuePath.getWildcardedQueuePaths(maxAutoCreatedQueueDepth);
+
+ for (QueuePath templateQueuePath: wildcardedQueuePaths) {
+ // Get all configuration entries with
+ // yarn.scheduler.capacity.<queuePath> prefix
+ Map<String, String> queueProps = configurationProperties
+ .getPropertiesWithPrefix(getQueuePrefix(templateQueuePath));
+
+ // Store template, parent-template and leaf-template properties
+ for (Map.Entry<String, String> entry : queueProps.entrySet()) {
+ storeConfiguredTemplates(entry.getKey(), entry.getValue());
+ }
Review Comment:
I can see the logic has been moved to QueuePath here.
Is this thoroughly tested with unit tests?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1560,82 +1545,82 @@ void setWorkflowPriorityMappings(
public boolean isReservable(String queue) {
boolean isReservable =
- getBoolean(getQueuePrefix(queue) + IS_RESERVABLE, false);
+ getBoolean(getQueuePrefix(new QueuePath(queue)) + IS_RESERVABLE,
false);
return isReservable;
}
public void setReservable(String queue, boolean isReservable) {
- setBoolean(getQueuePrefix(queue) + IS_RESERVABLE, isReservable);
+ setBoolean(getQueuePrefix(new QueuePath(queue)) + IS_RESERVABLE,
isReservable);
Review Comment:
Please only create the QueuePath object once in this method.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1560,82 +1545,82 @@ void setWorkflowPriorityMappings(
public boolean isReservable(String queue) {
boolean isReservable =
- getBoolean(getQueuePrefix(queue) + IS_RESERVABLE, false);
+ getBoolean(getQueuePrefix(new QueuePath(queue)) + IS_RESERVABLE,
false);
return isReservable;
}
public void setReservable(String queue, boolean isReservable) {
- setBoolean(getQueuePrefix(queue) + IS_RESERVABLE, isReservable);
+ setBoolean(getQueuePrefix(new QueuePath(queue)) + IS_RESERVABLE,
isReservable);
LOG.debug("here setReservableQueue: queuePrefix={}, isReservableQueue={}",
- getQueuePrefix(queue), isReservable(queue));
+ getQueuePrefix(new QueuePath(queue)), isReservable(queue));
}
@Override
public long getReservationWindow(String queue) {
long reservationWindow =
- getLong(getQueuePrefix(queue) + RESERVATION_WINDOW,
+ getLong(getQueuePrefix(new QueuePath(queue)) + RESERVATION_WINDOW,
DEFAULT_RESERVATION_WINDOW);
return reservationWindow;
}
@Override
public float getAverageCapacity(String queue) {
float avgCapacity =
- getFloat(getQueuePrefix(queue) + AVERAGE_CAPACITY,
+ getFloat(getQueuePrefix(new QueuePath(queue)) + AVERAGE_CAPACITY,
MAXIMUM_CAPACITY_VALUE);
return avgCapacity;
}
@Override
public float getInstantaneousMaxCapacity(String queue) {
float instMaxCapacity =
- getFloat(getQueuePrefix(queue) + INSTANTANEOUS_MAX_CAPACITY,
+ getFloat(getQueuePrefix(new QueuePath(queue)) +
INSTANTANEOUS_MAX_CAPACITY,
MAXIMUM_CAPACITY_VALUE);
return instMaxCapacity;
}
- public void setInstantaneousMaxCapacity(String queue, float instMaxCapacity)
{
+ public void setInstantaneousMaxCapacity(QueuePath queue, float
instMaxCapacity) {
setFloat(getQueuePrefix(queue) + INSTANTANEOUS_MAX_CAPACITY,
instMaxCapacity);
}
- public void setReservationWindow(String queue, long reservationWindow) {
+ public void setReservationWindow(QueuePath queue, long reservationWindow) {
setLong(getQueuePrefix(queue) + RESERVATION_WINDOW, reservationWindow);
}
- public void setAverageCapacity(String queue, float avgCapacity) {
+ public void setAverageCapacity(QueuePath queue, float avgCapacity) {
setFloat(getQueuePrefix(queue) + AVERAGE_CAPACITY, avgCapacity);
}
@Override
public String getReservationAdmissionPolicy(String queue) {
Review Comment:
I noticed the same pattern for setReservationAgent / getReservationAgent as
well. Please revisit these and try to make getters / setters consistent in
terms of parameter types.
--
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]
