szilard-nemeth commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765028138
##########
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
##########
@@ -59,6 +60,15 @@ public QueuePath(String fullPath) {
setFromFullPath(fullPath);
}
+ /**
+ * Concatenate queue path parts into one queue path string.
+ * @param parts Parts of the full queue pathAutoCreatedQueueTemplate
+ * @return full path of the given queue parts
+ */
+ public static String concatenatePath(String... parts) {
Review comment:
This method is unused. Please remove it.
##########
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/AutoCreatedQueueTemplate.java
##########
@@ -18,15 +18,15 @@
package org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity;
+import org.apache.commons.collections.IteratorUtils;
Review comment:
Nit: Unused import, please remove
##########
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
##########
@@ -2189,6 +2189,11 @@ public String
getAutoCreatedQueueTemplateConfPrefix(String queuePath) {
return queuePath + DOT + AUTO_CREATED_LEAF_QUEUE_TEMPLATE_PREFIX;
}
+ @Private
+ public QueuePath getAutoCreatedQueueObjectTemplateConfPrefix(String
queuePath) {
+ return new QueuePath(queuePath, AUTO_CREATED_LEAF_QUEUE_TEMPLATE_PREFIX);
Review comment:
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?
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestAbsoluteResourceWithAutoQueue.java
##########
@@ -96,15 +96,15 @@ public void setUp() throws Exception {
private CapacitySchedulerConfiguration setupMinMaxResourceConfiguration(
CapacitySchedulerConfiguration csConf) {
// Update min/max resource to queueA/B/C
- csConf.setMinimumResourceRequirement("", QUEUEA_FULL, QUEUE_A_MINRES);
- csConf.setMinimumResourceRequirement("", QUEUEB_FULL, QUEUE_B_MINRES);
- csConf.setMinimumResourceRequirement("", QUEUEC_FULL, QUEUE_C_MINRES);
- csConf.setMinimumResourceRequirement("", QUEUED_FULL, QUEUE_D_MINRES);
-
- csConf.setMaximumResourceRequirement("", QUEUEA_FULL, QUEUE_A_MAXRES);
- csConf.setMaximumResourceRequirement("", QUEUEB_FULL, QUEUE_B_MAXRES);
- csConf.setMaximumResourceRequirement("", QUEUEC_FULL, QUEUE_C_MAXRES);
- csConf.setMaximumResourceRequirement("", QUEUED_FULL, QUEUE_D_MAXRES);
+ csConf.setMinimumResourceRequirement("", new QueuePath(QUEUEA_FULL),
QUEUE_A_MINRES);
Review comment:
Can we use the QueuePath object for the QUEUE*_FULL fields as well?
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestQueuePath.java
##########
@@ -54,6 +54,20 @@ public void testEmptyPart() {
Assert.assertFalse(queuePathWithoutEmptyPart.hasEmptyPart());
}
+ @Test
+ public void testNullPath() {
+ QueuePath queuePathWithNullPath = new QueuePath(null);
+ String parentOfQueueWithNullPath = queuePathWithNullPath.getParent();
+ String leafOfQueueWithNullPath = queuePathWithNullPath.getLeafName();
+ String fullPathOfQueueWithNullPath = queuePathWithNullPath.getFullPath();
+ boolean isTheQueueWithNullPathRoot = queuePathWithNullPath.isRoot();
+
+ Assert.assertEquals(parentOfQueueWithNullPath, null);
+ Assert.assertEquals(leafOfQueueWithNullPath, "");
+ Assert.assertEquals(fullPathOfQueueWithNullPath, "");
+ Assert.assertFalse(isTheQueueWithNullPathRoot);
Review comment:
Nit: These could be a more clear without the local variables, you can
inline all of them in the assertXX calls.
##########
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/AutoCreatedQueueTemplate.java
##########
@@ -155,14 +155,13 @@ public void
setTemplateEntriesForChild(CapacitySchedulerConfiguration conf,
* yarn.scheduler.capacity.root.*.auto-queue-creation-v2.template.capacity
*/
private void setTemplateConfigEntries(CapacitySchedulerConfiguration
configuration,
Review comment:
@brumi1024 @9uapaw Thoughts?
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestAbsoluteResourceConfiguration.java
##########
@@ -141,32 +141,32 @@ private CapacitySchedulerConfiguration
setupComplexQueueConfiguration(
private CapacitySchedulerConfiguration setupMinMaxResourceConfiguration(
CapacitySchedulerConfiguration csConf) {
// Update min/max resource to queueA/B/C
- csConf.setMinimumResourceRequirement("", QUEUEA_FULL, QUEUE_A_MINRES);
- csConf.setMinimumResourceRequirement("", QUEUEB_FULL, QUEUE_B_MINRES);
- csConf.setMinimumResourceRequirement("", QUEUEC_FULL, QUEUE_C_MINRES);
- csConf.setMinimumResourceRequirement("", QUEUED_FULL, QUEUE_D_MINRES);
+ csConf.setMinimumResourceRequirement("", new QueuePath(QUEUEA_FULL),
QUEUE_A_MINRES);
Review comment:
Can we use the QueuePath object for these fields as well:
- QUEUEA1_FULL
- QUEUEA2_FULL
- QUEUEB1_FULL
?
##########
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/AutoCreatedQueueTemplate.java
##########
@@ -155,14 +155,13 @@ public void
setTemplateEntriesForChild(CapacitySchedulerConfiguration conf,
* yarn.scheduler.capacity.root.*.auto-queue-creation-v2.template.capacity
*/
private void setTemplateConfigEntries(CapacitySchedulerConfiguration
configuration,
Review comment:
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.
##########
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 +89,12 @@
*/
public String getQueuePath();
+ /**
+ * Get the object of the queue.
Review comment:
```suggestion
* Gets the queue path object.
```
##########
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
##########
@@ -176,7 +174,7 @@ protected void setupConfigurableCapacities() {
protected void setupConfigurableCapacities(
CapacitySchedulerConfiguration configuration) {
- CSQueueUtils.loadCapacitiesByLabelsFromConf(getQueuePath(),
queueCapacities,
+ CSQueueUtils.loadCapacitiesByLabelsFromConf(getQueuePathObject(),
queueCapacities,
Review comment:
No need to use getQueuePathObject getter inside this class, you can use
the field directly.
This applies to all getQueuePathObject() calls from this class
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerConfigValidator.java
##########
@@ -225,7 +225,8 @@ public void
testValidateCSConfigDefaultRCAbsoluteModeParentMaxMemoryExceeded()
CapacitySchedulerConfiguration oldConfiguration = cs.getConfiguration();
CapacitySchedulerConfiguration newConfiguration =
new CapacitySchedulerConfiguration(cs.getConfiguration());
- newConfiguration.setMaximumResourceRequirement("", LEAF_A_FULL_PATH,
FULL_MAXRES);
Review comment:
I think these fields should be QueuePath objects:
```
private static final String PARENT_A_FULL_PATH =
CapacitySchedulerConfiguration.ROOT
+ "." + PARENT_A;
private static final String LEAF_A_FULL_PATH = PARENT_A_FULL_PATH
+ "." + LEAF_A;
private static final String PARENT_B_FULL_PATH =
CapacitySchedulerConfiguration.ROOT
+ "." + PARENT_B;
private static final String LEAF_B_FULL_PATH = PARENT_B_FULL_PATH
+ "." + LEAF_B;
```
##########
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
##########
@@ -527,10 +527,10 @@ private void throwExceptionForUnexpectedWeight(float
weight, String queue,
}
}
- public float getNonLabeledQueueWeight(String queue) {
- String configuredValue = get(getQueuePrefix(queue) + CAPACITY);
+ public float getNonLabeledQueueWeight(QueuePath queue) {
+ String configuredValue = get(getQueuePrefix(queue.getFullPath()) +
CAPACITY);
Review comment:
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.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestAutoCreatedQueueTemplate.java
##########
@@ -45,46 +45,46 @@ public void setUp() throws Exception {
public void testNonWildCardTemplate() {
conf.set(getTemplateKey(TEST_QUEUE_AB, "capacity"), "6w");
AutoCreatedQueueTemplate template =
- new AutoCreatedQueueTemplate(conf, TEST_QUEUE_AB);
Review comment:
Can we use the QueuePath object to replace the following fields as well?
```
private static final String TEST_QUEUE_ABC = "root.a.b.c";
private static final String TEST_QUEUE_AB = "root.a.b";
private static final String TEST_QUEUE_A = "root.a";
private static final String TEST_QUEUE_B = "root.b";
```
##########
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
##########
@@ -121,6 +136,14 @@ public boolean hasParent() {
return parent != null;
}
+ /**
+ * Convenience getter to check if the queue is a root path.
Review comment:
```suggestion
* Convenience getter to check if the queue is the root queue.
```
##########
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
##########
@@ -289,7 +292,7 @@ public void verifyClusterSchedulerXML(NodeList nodes)
throws Exception {
}
}
- public void verifySubQueueXML(Element qElem, String q,
+ public void verifySubQueueXML(Element qElem, String queue,
Review comment:
I think all the changes in this class can be thrown away as the
assertion of JSON/XML responses has changed with:
https://issues.apache.org/jira/browse/YARN-11031
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacityScheduler.java
##########
@@ -677,11 +677,13 @@ private void nodeUpdate(NodeManager nm) {
public void testMaximumCapacitySetup() {
float delta = 0.0000001f;
CapacitySchedulerConfiguration conf = new CapacitySchedulerConfiguration();
-
assertEquals(CapacitySchedulerConfiguration.MAXIMUM_CAPACITY_VALUE,conf.getNonLabeledQueueMaximumCapacity(A),delta);
+ assertEquals(CapacitySchedulerConfiguration.MAXIMUM_CAPACITY_VALUE,
+ conf.getNonLabeledQueueMaximumCapacity(new QueuePath(A)), delta);
conf.setMaximumCapacity(A, 50.0f);
- assertEquals(50.0f, conf.getNonLabeledQueueMaximumCapacity(A),delta);
+ assertEquals(50.0f, conf.getNonLabeledQueueMaximumCapacity(new
QueuePath(A)), delta);
Review comment:
I think these fields should be QueuePath objects:
```
public static final String A = CapacitySchedulerConfiguration.ROOT + ".a";
public static final String B = CapacitySchedulerConfiguration.ROOT + ".b";
public static final String A1 = A + ".a1";
public static final String A2 = A + ".a2";
public static final String B1 = B + ".b1";
public static final String B2 = B + ".b2";
public static final String B3 = B + ".b3";
public static final float A_CAPACITY = 10.5f;
public static final float B_CAPACITY = 89.5f;
public static final String P1 = CapacitySchedulerConfiguration.ROOT +
".p1";
public static final String P2 = CapacitySchedulerConfiguration.ROOT +
".p2";
public static final String X1 = P1 + ".x1";
public static final String X2 = P1 + ".x2";
public static final String Y1 = P2 + ".y1";
public static final String Y2 = P2 + ".y2";
```
Rather than making a QueuePath object from "A" all the time.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestParentQueue.java
##########
@@ -131,10 +131,10 @@ private void setupSingleLevelQueuesWithAbsoluteResource(
// Define top-level queues
conf.setQueues(CapacitySchedulerConfiguration.ROOT, new String[]{A, B});
- conf.setMinimumResourceRequirement("", Q_A,
Review comment:
I think these fields should be QueuePath objects:
```
private static final String Q_A =
CapacitySchedulerConfiguration.ROOT + "." + A;
private static final String Q_B =
CapacitySchedulerConfiguration.ROOT + "." + B;
```
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestQueuePath.java
##########
@@ -54,6 +54,20 @@ public void testEmptyPart() {
Assert.assertFalse(queuePathWithoutEmptyPart.hasEmptyPart());
}
+ @Test
+ public void testNullPath() {
+ QueuePath queuePathWithNullPath = new QueuePath(null);
+ String parentOfQueueWithNullPath = queuePathWithNullPath.getParent();
+ String leafOfQueueWithNullPath = queuePathWithNullPath.getLeafName();
+ String fullPathOfQueueWithNullPath = queuePathWithNullPath.getFullPath();
+ boolean isTheQueueWithNullPathRoot = queuePathWithNullPath.isRoot();
+
+ Assert.assertEquals(parentOfQueueWithNullPath, null);
Review comment:
You can use assertNull here.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServicesConfigurationMutation.java
##########
@@ -292,9 +293,9 @@ public void testAddNestedQueue() throws Exception {
((CapacityScheduler) rm.getResourceScheduler()).getConfiguration();
assertEquals(4, newCSConf.getQueues("root").length);
Review comment:
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.
--
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]
