You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2021/11/16 10:18:04 UTC
[GitHub] [hadoop] 9uapaw commented on a change in pull request #3660: YARN-10982
9uapaw commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r750050820
##########
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/AbstractCSQueue.java
##########
@@ -85,8 +85,9 @@
private static final Logger LOG =
LoggerFactory.getLogger(AbstractCSQueue.class);
volatile CSQueue parent;
- final String queueName;
- private final String queuePath;
+// final String queueName;
+// private final String queuePath;
Review comment:
Comments left here.
##########
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/AbstractCSQueue.java
##########
@@ -416,7 +420,7 @@ protected void setDynamicQueueProperties(
if (parentNodeLabels != null && parentNodeLabels.size() > 1) {
csContext.getCapacitySchedulerQueueManager().getConfiguredNodeLabels()
- .setLabelsByQueue(queuePath, new HashSet<>(parentNodeLabels));
+ .setLabelsByQueue(this.queuePath.getFullPath(), new HashSet<>(parentNodeLabels));
Review comment:
Do not use this identifier if not necessary.
##########
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/CSQueue.java
##########
@@ -89,6 +84,8 @@
*/
public String getQueuePath();
+ public QueuePath getQueuePathObject();
Review comment:
Please add documentation here.
##########
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/AbstractCSQueue.java
##########
@@ -159,9 +160,11 @@ public AbstractCSQueue(CapacitySchedulerContext cs,
this.labelManager = cs.getRMContext().getNodeLabelManager();
this.parent = parent;
- this.queueName = queueName;
- this.queuePath = ((parent == null) ? "" : (parent.getQueuePath() + "."))
- + this.queueName;
+// this.queueName = queueName;
+// this.queuePath = ((parent == null) ? "" : (parent.getQueuePath() + "."))
+// + this.queueName;
Review comment:
Comments left here.
##########
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/QueuePath.java
##########
@@ -18,12 +18,10 @@
package org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity;
-import java.util.Arrays;
-import java.util.Iterator;
-import java.util.NoSuchElementException;
-import java.util.Objects;
+import java.util.*;
Review comment:
Do not use wildcard imports.
##########
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/CapacitySchedulerConfiguration.java
##########
@@ -807,9 +808,9 @@ private float extractFloatValueFromWeightConfig(String configureValue) {
}
}
- private float internalGetLabeledQueueCapacity(String queue, String label,
+ private float internalGetLabeledQueueCapacity(QueuePath queue, String label,
Review comment:
Lets have a discussion about changing parameters in CSConfig. I see little value here apart from the isRoot, but a lot of tests use these methods and this would change them 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/AbstractCSQueue.java
##########
@@ -649,7 +653,7 @@ private void validateMinResourceIsNotGreaterThanMaxResource(Resource minResource
private void validateAbsoluteVsPercentageCapacityConfig(
CapacityConfigType localType) {
- if (!queuePath.equals("root")
+ if (!queuePath.getFullPath().equals("root")
Review comment:
This logic could be extracted to QueuePath class and used as queuePath.isRoot()
##########
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/CSQueue.java
##########
@@ -38,12 +38,7 @@
import org.apache.hadoop.yarn.security.PrivilegedEntity;
import org.apache.hadoop.yarn.server.resourcemanager.rmcontainer.RMContainer;
import org.apache.hadoop.yarn.server.resourcemanager.rmcontainer.RMContainerEventType;
-import org.apache.hadoop.yarn.server.resourcemanager.scheduler.AbstractUsersManager;
-import org.apache.hadoop.yarn.server.resourcemanager.scheduler.QueueResourceQuotas;
-import org.apache.hadoop.yarn.server.resourcemanager.scheduler.ResourceLimits;
-import org.apache.hadoop.yarn.server.resourcemanager.scheduler.ResourceUsage;
-import org.apache.hadoop.yarn.server.resourcemanager.scheduler.SchedulerApplicationAttempt;
-import org.apache.hadoop.yarn.server.resourcemanager.scheduler.SchedulerQueue;
+import org.apache.hadoop.yarn.server.resourcemanager.scheduler.*;
Review comment:
Do not use wildcard imports.
##########
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/AbstractCSQueue.java
##########
@@ -159,9 +160,11 @@ public AbstractCSQueue(CapacitySchedulerContext cs,
this.labelManager = cs.getRMContext().getNodeLabelManager();
this.parent = parent;
- this.queueName = queueName;
- this.queuePath = ((parent == null) ? "" : (parent.getQueuePath() + "."))
- + this.queueName;
+// this.queueName = queueName;
+// this.queuePath = ((parent == null) ? "" : (parent.getQueuePath() + "."))
+// + this.queueName;
+ String fullQueuePath = ((parent == null) ? "" : (parent.getQueuePath() + ".")) + queueName;
Review comment:
This logic is already contained in QueuePath.
##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServicesCapacitySched.java
##########
@@ -460,7 +461,7 @@ private void verifySubQueue(JSONObject info, String q,
// test subqueues
for (int i = 0; i < arr.length(); i++) {
JSONObject obj = arr.getJSONObject(i);
- String q2 = q + "." + obj.getString("queueName");
+ String q2 = queue + "." + obj.getString("queueName");
Review comment:
This is an append action aka. a parent + leaf queue path construction.
##########
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/CapacitySchedulerConfiguration.java
##########
@@ -2180,6 +2181,11 @@ public String getAutoCreatedQueueTemplateConfPrefix(String queuePath) {
return queuePath + DOT + AUTO_CREATED_LEAF_QUEUE_TEMPLATE_PREFIX;
}
+ @Private
+ public QueuePath getAutoCreatedQueueTemplateConfPrefix_NEW(String queuePath) {
Review comment:
The name of the method does not adhere to the conventions used in Java.
##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServicesCapacitySched.java
##########
@@ -488,22 +489,22 @@ private void verifySubQueue(JSONObject info, String q,
lqi.defaultApplicationLifetime =
info.getLong("defaultApplicationLifetime");
lqi.maxApplicationLifetime = info.getLong("maxApplicationLifetime");
- verifyLeafQueueGeneric(q, lqi);
+ verifyLeafQueueGeneric(queue, lqi);
// resourcesUsed and users (per-user resources used) are checked in
// testPerUserResource()
}
}
- private void verifySubQueueGeneric(String q, QueueInfo info,
+ private void verifySubQueueGeneric(QueuePath queue, QueueInfo info,
float parentAbsCapacity, float parentAbsMaxCapacity) throws Exception {
- String[] qArr = q.split("\\.");
- assertTrue("q name invalid: " + q, qArr.length > 1);
+ String[] qArr = queue.getFullPath().split("\\.");
Review comment:
This is the same as the queue path parts.
##########
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/QueuePath.java
##########
@@ -68,6 +66,10 @@ private void setFromFullPath(String fullPath) {
parent = null;
leaf = fullPath;
+ if (leaf == null) {
+ leaf = "";
+ }
+
int lastDotIdx = fullPath.lastIndexOf(DOT);
Review comment:
fullPath could still be null here. lastDotIdx = leaf.lastIndexOf(DOT); should be the correct way to handle this corner case. Also please add a test case for the scenario.
##########
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/CapacitySchedulerConfiguration.java
##########
@@ -2556,20 +2562,21 @@ public Resource getMaximumResourceRequirement(String label, String queue,
}
@VisibleForTesting
- public void setMinimumResourceRequirement(String label, String queue,
+ public void setMinimumResourceRequirement(String label, QueuePath queue,
Resource resource) {
updateMinMaxResourceToConf(label, queue, resource, CAPACITY);
}
@VisibleForTesting
- public void setMaximumResourceRequirement(String label, String queue,
+ public void setMaximumResourceRequirement(String label, QueuePath queue,
Resource resource) {
updateMinMaxResourceToConf(label, queue, resource, MAXIMUM_CAPACITY);
}
- private void updateMinMaxResourceToConf(String label, String queue,
+ private void updateMinMaxResourceToConf(String label, QueuePath queue,
Resource resource, String type) {
- if (queue.equals("root")) {
+// if (queue.equals("root")) {
Review comment:
Comment is left here.
##########
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/QueuePath.java
##########
@@ -131,6 +139,15 @@ public QueuePath createNewLeaf(String childQueue) {
return new QueuePath(getFullPath(), childQueue);
}
+ /**
+ * Creates a new {@code List<String>} from the current full path as parent, split
+ * by the comma separator.
+ * @return new array list with the parts of the full path split by commas
+ */
+ public List<String> getPathParts() {
+ return new ArrayList<>(Arrays.asList(getFullPath().split(QUEUE_REGEX_DELIMITER)));
Review comment:
This logic is implemented in the iterator. Please us this logic there as well.
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org