You are viewing a plain text version of this content. The canonical link for it is here.
Posted to yarn-issues@hadoop.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2023/05/02 00:27:00 UTC
[jira] [Commented] (YARN-11041) Replace all occurences of queuePath with the new QueuePath class - followup
[ https://issues.apache.org/jira/browse/YARN-11041?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17718381#comment-17718381 ]
ASF GitHub Bot commented on YARN-11041:
---------------------------------------
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.
> Replace all occurences of queuePath with the new QueuePath class - followup
> ---------------------------------------------------------------------------
>
> Key: YARN-11041
> URL: https://issues.apache.org/jira/browse/YARN-11041
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: capacity scheduler
> Reporter: Tibor Kovács
> Assignee: Peter Szucs
> Priority: Major
> Labels: pull-request-available
> Fix For: 3.4.0
>
>
> The QueuePath class was introduced in YARN-10897, however, its current adoption happened only for code changes after this JIRA. We need to adopt it retrospectively.
>
> A lot of changes are introduced via ticket YARN-10982. The replacing should be continued by touching the next comments:
>
> [...g/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AutoCreatedQueueTemplate.java|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-fde6885144b59bb06b2c3358780388d958829b13f68aceee7bb6d394bb5e0548]
> |[~snemeth] [https://github.com/apache/hadoop/pull/3660#discussion_r765012937]
> I think this could be also refactored in a follow-up jira so the string magic could probably be replaced with some more elegant solution. Though, I think this would be too much in this patch, hence I do suggest the follow-up jira.|
> |[~snemeth] [https://github.com/apache/hadoop/pull/3660#discussion_r765013096]
> [~bteke] [ |https://github.com/9uapaw] [~gandras] [ \|https://github.com/9uapaw] Thoughts?|
> |[~bteke] [https://github.com/apache/hadoop/pull/3660#discussion_r765110750]
> +1, even the QueuePath object could have some kind of support for this.|
> |[~gandras] [https://github.com/apache/hadoop/pull/3660#discussion_r765131244]
> Agreed, let's handle it in a followup!|
>
> ----
>
> [...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-c4b0c5e70208f1e3cfbd5a86ffa2393e5c996cc8b45605d9d41abcb7e0bd382a]
> |[~snemeth] [https://github.com/apache/hadoop/pull/3660#discussion_r765023717]
> There are many string operations in this class:
> E.g. * getQueuePrefix that works with the full queue path
> * getNodeLabelPrefix that also works with the full queue path|
> I suggest to create a static class, called "QueuePrefixes" or something like that and add some static methods there to convert the QueuePath object to those various queue prefix strings that are ultimately keys in the Configuration object.
>
> ----
>
> [...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-c4b0c5e70208f1e3cfbd5a86ffa2393e5c996cc8b45605d9d41abcb7e0bd382a]
> |[~snemeth] [https://github.com/apache/hadoop/pull/3660#discussion_r765026119]
> This seems hacky, just based on the constructor parameter names of QueuePath: parent, leaf.
> The AQC Template prefix is not the leaf, obviously.
> Could we somehow circumvent this?|
> |[~bteke] [https://github.com/apache/hadoop/pull/3660#discussion_r765126207]
> Maybe a factory method could be created, which returns a new QueuePath with the parent set as the original queuePath. I.e rootQueuePath.createChild(String childName) -> this could return a new QueuePath object with root.childName path, and rootQueuePath as parent.|
> |[~snemeth] [https://github.com/apache/hadoop/pull/3660#discussion_r765039033]
> Looking at this getQueues method, I realized almost all the callers are using some kind of string magic that should be addressed with this patch.
> For example, take a look at: org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.conf.MutableCSConfigurationProvider#addQueue
> I think getQueues should also receive the QueuePath object instead of Strings.|
>
> ----
>
> [.../src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CSQueue.java|https://github.com/apache/hadoop/pull/3660/files/0c3dd17c936260fc9c386dcabc6368b54b27aa82..39f4ec203377244f840e4593aa02386ff51cc3c4#diff-0adf8192c51cbe4671324f06f7f8cbd48898df0376bbcc516451a3bdb2b48d3b]
> |[~bteke] [https://github.com/apache/hadoop/pull/3660#discussion_r765912967]
> Nit: Gets the queue path object.
> The object of the queue suggests a CSQueue object.|
> |[~snemeth] [https://github.com/apache/hadoop/pull/3660#discussion_r765922133]
> Will fix the nit upon commit if I'm fine with the whole patch. Thanks for noticing.|
>
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org