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/15 10:29:10 UTC
[GitHub] [hadoop] TiborKovacsCloudera opened a new pull request #3660: YARN-10982
TiborKovacsCloudera opened a new pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660
Use QueuePath class where it's reasonable.
--
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
[GitHub] [hadoop] hadoop-yetus commented on pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#issuecomment-990197529
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 37s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 9 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 33m 43s | | trunk passed |
| +1 :green_heart: | compile | 1m 4s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | compile | 0m 57s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | checkstyle | 0m 51s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 3s | | trunk passed |
| +1 :green_heart: | javadoc | 0m 50s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 0m 45s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 11s | | trunk passed |
| +1 :green_heart: | shadedclient | 22m 10s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 0m 57s | | the patch passed |
| +1 :green_heart: | compile | 1m 1s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javac | 1m 1s | | the patch passed |
| +1 :green_heart: | compile | 0m 52s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | javac | 0m 52s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 41s | | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 341 unchanged - 8 fixed = 341 total (was 349) |
| +1 :green_heart: | mvnsite | 0m 58s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 43s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 0m 37s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 6s | | the patch passed |
| +1 :green_heart: | shadedclient | 21m 24s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 96m 20s | | hadoop-yarn-server-resourcemanager in the patch passed. |
| +1 :green_heart: | asflicense | 0m 34s | | The patch does not generate ASF License warnings. |
| | | 189m 12s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/7/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/3660 |
| JIRA Issue | YARN-10982 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux 3c12e2a1c0a9 4.15.0-156-generic #163-Ubuntu SMP Thu Aug 19 23:31:58 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / fb681a9a45e476f2059d2d93bba0d04756cc1e72 |
| Default Java | Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/7/testReport/ |
| Max. process+thread count | 932 (vs. ulimit of 5500) |
| modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/7/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
--
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
[GitHub] [hadoop] TiborKovacsCloudera commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
TiborKovacsCloudera commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765657347
##########
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:
To have a QueuePath from Q_A and Q_B are only in this method is necessary. So the only occurrence for this purpose is only one.
Q_A and Q_B are used as string in setCapacity(), setQueues() and set() methods.
--
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
[GitHub] [hadoop] szilard-nemeth commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
szilard-nemeth commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765922133
##########
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
##########
@@ -90,7 +90,7 @@
public String getQueuePath();
/**
- * Get the object of the queue.
+ * Gets the object of the queue.
Review comment:
Will fix the nit upon commit if I'm fine with the whole patch. Thanks for noticing.
--
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
[GitHub] [hadoop] szilard-nemeth commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
szilard-nemeth commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765965751
##########
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:
Yep, there is no caller of this. Please make sure to add this to the follow-up jira's description 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
[GitHub] [hadoop] 9uapaw commented on a change in pull request #3660: YARN-10982
Posted by GitBox <gi...@apache.org>.
9uapaw commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r755099357
##########
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
##########
@@ -332,7 +336,7 @@ public void verifySubQueueXML(Element qElem, String q,
for (int k = 0; k < qListInfos.getLength(); k++) {
Element qElem3 = (Element) qListInfos.item(k);
String qName3 = WebServicesTestUtils.getXmlString(qElem3, "queueName");
- String q3 = q + "." + qName3;
+ String q3 = queue + "." + qName3;
Review comment:
It is an append operation, defined for QueuePath.
##########
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/CSQueueUtils.java
##########
@@ -36,23 +36,23 @@
/*
* Used only by tests
*/
- public static void checkMaxCapacity(String queuePath,
+ public static void checkMaxCapacity(QueuePath queuePath,
float capacity, float maximumCapacity) {
if (maximumCapacity < 0.0f || maximumCapacity > 1.0f) {
throw new IllegalArgumentException(
"Illegal value of maximumCapacity " + maximumCapacity +
- " used in call to setMaxCapacity for queue " + queuePath);
+ " used in call to setMaxCapacity for queue " + queuePath.getFullPath());
Review comment:
Calling getFullPath is superfluous, as QueuePath defines toString.
##########
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
##########
@@ -834,17 +835,17 @@ private float internalGetLabeledQueueCapacity(String queue, String label,
}
if (LOG.isDebugEnabled()) {
LOG.debug(
- "CSConf - getCapacityOfLabel: prefix=" + getNodeLabelPrefix(queue,
+ "CSConf - getCapacityOfLabel: prefix=" + getNodeLabelPrefix(queue.getFullPath(),
Review comment:
Same 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/CSQueueUtils.java
##########
@@ -36,23 +36,23 @@
/*
* Used only by tests
*/
- public static void checkMaxCapacity(String queuePath,
+ public static void checkMaxCapacity(QueuePath queuePath,
float capacity, float maximumCapacity) {
if (maximumCapacity < 0.0f || maximumCapacity > 1.0f) {
throw new IllegalArgumentException(
"Illegal value of maximumCapacity " + maximumCapacity +
- " used in call to setMaxCapacity for queue " + queuePath);
+ " used in call to setMaxCapacity for queue " + queuePath.getFullPath());
}
}
/*
* Used only by tests
*/
- public static void checkAbsoluteCapacity(String queuePath,
+ public static void checkAbsoluteCapacity(QueuePath queuePath,
float absCapacity, float absMaxCapacity) {
if (absMaxCapacity < (absCapacity - EPSILON)) {
throw new IllegalArgumentException("Illegal call to setMaxCapacity. "
- + "Queue '" + queuePath + "' has "
+ + "Queue '" + queuePath.getFullPath() + "' has "
Review comment:
Same here.
##########
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();
Review comment:
No need to Box here, use boolean instead.
--
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
[GitHub] [hadoop] TiborKovacsCloudera commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
TiborKovacsCloudera commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765651163
##########
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:
Q_A as a QueuePath object is used only here. Same with Q_B. These variables are used in setCapacity(), setQueues() and set() methods.
--
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
[GitHub] [hadoop] brumi1024 commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
brumi1024 commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765126207
##########
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:
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.
--
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
[GitHub] [hadoop] brumi1024 commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
brumi1024 commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765912967
##########
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
##########
@@ -90,7 +90,7 @@
public String getQueuePath();
/**
- * Get the object of the queue.
+ * Gets the object of the queue.
Review comment:
Nit: Gets the queue path object.
The object of the queue suggests a CSQueue object.
--
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
[GitHub] [hadoop] szilard-nemeth closed pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
szilard-nemeth closed pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660
--
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
[GitHub] [hadoop] szilard-nemeth commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
szilard-nemeth commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765964349
##########
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:
I don't see all of these fixed in this class. Anyway, let's go with the current version.
--
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
[GitHub] [hadoop] szilard-nemeth commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
szilard-nemeth commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765698078
##########
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:
I think that code fragment has been removed.
--
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
[GitHub] [hadoop] 9uapaw commented on a change in pull request #3660: YARN-10982
Posted by GitBox <gi...@apache.org>.
9uapaw commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r759127661
##########
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
##########
@@ -332,7 +336,7 @@ public void verifySubQueueXML(Element qElem, String q,
for (int k = 0; k < qListInfos.getLength(); k++) {
Element qElem3 = (Element) qListInfos.item(k);
String qName3 = WebServicesTestUtils.getXmlString(qElem3, "queueName");
- String q3 = q + "." + qName3;
+ String q3 = queue + "." + qName3;
Review comment:
Lets make a static utility function on QueuePath then, as the following:
```java
public static String concatenatePath(String... parts) {
return String.join(DOT, parts);
}
```
This will be useful for testing purposes 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
[GitHub] [hadoop] TiborKovacsCloudera commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
TiborKovacsCloudera commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765638646
##########
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:
It is not necessary to change the type all of it. And it isn't worth to change type A. The only occurrence of A in this class is in this method.
--
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
[GitHub] [hadoop] TiborKovacsCloudera commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
TiborKovacsCloudera commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765683094
##########
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:
A followup Jira is created: https://issues.apache.org/jira/browse/YARN-11041
--
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
[GitHub] [hadoop] szilard-nemeth commented on pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
szilard-nemeth commented on pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#issuecomment-990031161
Thanks @TiborKovacsCloudera for working on this.
Latest patch LGTM, committed to trunk.
Thanks @9uapaw and @brumi1024 for the reviews.
--
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
[GitHub] [hadoop] 9uapaw commented on a change in pull request #3660: YARN-10982
Posted by GitBox <gi...@apache.org>.
9uapaw commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r759004909
##########
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
##########
@@ -834,17 +835,17 @@ private float internalGetLabeledQueueCapacity(String queue, String label,
}
if (LOG.isDebugEnabled()) {
LOG.debug(
- "CSConf - getCapacityOfLabel: prefix=" + getNodeLabelPrefix(queue,
+ "CSConf - getCapacityOfLabel: prefix=" + getNodeLabelPrefix(queue.getFullPath(),
Review comment:
Ahh my bad, never mind.
--
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
[GitHub] [hadoop] szilard-nemeth commented on pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
szilard-nemeth commented on pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#issuecomment-983938671
@TiborKovacsCloudera Hi,
For me the UI shows conflicts. Could you please check it?
--
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
[GitHub] [hadoop] TiborKovacsCloudera commented on a change in pull request #3660: YARN-10982
Posted by GitBox <gi...@apache.org>.
TiborKovacsCloudera commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r759027274
##########
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
##########
@@ -332,7 +336,7 @@ public void verifySubQueueXML(Element qElem, String q,
for (int k = 0; k < qListInfos.getLength(); k++) {
Element qElem3 = (Element) qListInfos.item(k);
String qName3 = WebServicesTestUtils.getXmlString(qElem3, "queueName");
- String q3 = q + "." + qName3;
+ String q3 = queue + "." + qName3;
Review comment:
q3 will use only once for a function which needs a string, therefore I think this code part is not necessary to be modified.
--
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
[GitHub] [hadoop] szilard-nemeth commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
szilard-nemeth commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765970056
##########
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:
OK, modified all the usages. Will commit with the updated version.
--
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
[GitHub] [hadoop] hadoop-yetus commented on pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#issuecomment-988563468
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 0s | | Docker mode activated. |
| -1 :x: | patch | 0m 18s | | https://github.com/apache/hadoop/pull/3660 does not apply to trunk. Rebase required? Wrong Branch? See https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute for help. |
| Subsystem | Report/Notes |
|----------:|:-------------|
| GITHUB PR | https://github.com/apache/hadoop/pull/3660 |
| JIRA Issue | YARN-10982 |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/2/console |
| versions | git=2.17.1 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
--
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
[GitHub] [hadoop] hadoop-yetus commented on pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#issuecomment-985609325
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 40s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 9 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 33m 11s | | trunk passed |
| +1 :green_heart: | compile | 1m 5s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | compile | 0m 58s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | checkstyle | 0m 47s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 7s | | trunk passed |
| +1 :green_heart: | javadoc | 0m 44s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 0m 46s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 10s | | trunk passed |
| +1 :green_heart: | shadedclient | 21m 45s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 0m 57s | | the patch passed |
| +1 :green_heart: | compile | 0m 56s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| -1 :x: | javac | 0m 56s | [/results-compile-javac-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkUbuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/1/artifact/out/results-compile-javac-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkUbuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04.txt) | hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkUbuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 generated 1 new + 54 unchanged - 1 fixed = 55 total (was 55) |
| +1 :green_heart: | compile | 0m 52s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | javac | 0m 52s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 0m 40s | [/results-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/1/artifact/out/results-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt) | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 20 new + 367 unchanged - 8 fixed = 387 total (was 375) |
| +1 :green_heart: | mvnsite | 0m 54s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 39s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| -1 :x: | javadoc | 0m 37s | [/results-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkPrivateBuild-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/1/artifact/out/results-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkPrivateBuild-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10.txt) | hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkPrivateBuild-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 generated 1 new + 344 unchanged - 0 fixed = 345 total (was 344) |
| +1 :green_heart: | spotbugs | 2m 2s | | the patch passed |
| +1 :green_heart: | shadedclient | 21m 49s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 97m 10s | | hadoop-yarn-server-resourcemanager in the patch passed. |
| +1 :green_heart: | asflicense | 0m 34s | | The patch does not generate ASF License warnings. |
| | | 189m 22s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/3660 |
| JIRA Issue | YARN-10982 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux 8fb2d1ed52d8 4.15.0-156-generic #163-Ubuntu SMP Thu Aug 19 23:31:58 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 8c232c2374447e80da4360f753814b6d6ef0cbe9 |
| Default Java | Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/1/testReport/ |
| Max. process+thread count | 950 (vs. ulimit of 5500) |
| modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/1/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
--
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
[GitHub] [hadoop] brumi1024 commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
brumi1024 commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765113653
##########
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:
Additionally assert has the order reversed: expected, actual. Can you please fix the parameter ordering?
--
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
[GitHub] [hadoop] hadoop-yetus commented on pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#issuecomment-988839290
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 46s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 10 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 35m 53s | | trunk passed |
| +1 :green_heart: | compile | 1m 10s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | compile | 1m 2s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | checkstyle | 0m 52s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 6s | | trunk passed |
| +1 :green_heart: | javadoc | 0m 54s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 0m 47s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 5s | | trunk passed |
| +1 :green_heart: | shadedclient | 24m 8s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 1s | | the patch passed |
| +1 :green_heart: | compile | 1m 4s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javac | 1m 4s | | the patch passed |
| +1 :green_heart: | compile | 0m 52s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | javac | 0m 52s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 0m 43s | [/results-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/4/artifact/out/results-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt) | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 1 new + 388 unchanged - 8 fixed = 389 total (was 396) |
| +1 :green_heart: | mvnsite | 0m 55s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 42s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 0m 40s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 6s | | the patch passed |
| +1 :green_heart: | shadedclient | 23m 55s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 96m 40s | | hadoop-yarn-server-resourcemanager in the patch passed. |
| +1 :green_heart: | asflicense | 0m 34s | | The patch does not generate ASF License warnings. |
| | | 197m 9s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/4/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/3660 |
| JIRA Issue | YARN-10982 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux 5ff5a3d63598 4.15.0-156-generic #163-Ubuntu SMP Thu Aug 19 23:31:58 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / f956918bc154d0e35fce07c5dd8be804eb007acc |
| Default Java | Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/4/testReport/ |
| Max. process+thread count | 940 (vs. ulimit of 5500) |
| modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/4/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
--
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
[GitHub] [hadoop] TiborKovacsCloudera commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
TiborKovacsCloudera commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765683184
##########
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:
A followup Jira is created: https://issues.apache.org/jira/browse/YARN-11041
##########
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:
A followup Jira is created: https://issues.apache.org/jira/browse/YARN-11041
##########
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:
A followup Jira is created: https://issues.apache.org/jira/browse/YARN-11041
--
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
[GitHub] [hadoop] 9uapaw commented on a change in pull request #3660: YARN-10982
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [hadoop] TiborKovacsCloudera commented on a change in pull request #3660: YARN-10982
Posted by GitBox <gi...@apache.org>.
TiborKovacsCloudera commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r759003326
##########
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
##########
@@ -834,17 +835,17 @@ private float internalGetLabeledQueueCapacity(String queue, String label,
}
if (LOG.isDebugEnabled()) {
LOG.debug(
- "CSConf - getCapacityOfLabel: prefix=" + getNodeLabelPrefix(queue,
+ "CSConf - getCapacityOfLabel: prefix=" + getNodeLabelPrefix(queue.getFullPath(),
Review comment:
getNodeLabelPrefix requires two 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: 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
[GitHub] [hadoop] TiborKovacsCloudera commented on a change in pull request #3660: YARN-10982
Posted by GitBox <gi...@apache.org>.
TiborKovacsCloudera commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r759027274
##########
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
##########
@@ -332,7 +336,7 @@ public void verifySubQueueXML(Element qElem, String q,
for (int k = 0; k < qListInfos.getLength(); k++) {
Element qElem3 = (Element) qListInfos.item(k);
String qName3 = WebServicesTestUtils.getXmlString(qElem3, "queueName");
- String q3 = q + "." + qName3;
+ String q3 = queue + "." + qName3;
Review comment:
q3 will use only once for a function which needs a string, therefore I think this code part is not necessary to modify.
--
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
[GitHub] [hadoop] hadoop-yetus commented on pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#issuecomment-988660935
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 45s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 9 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 34m 34s | | trunk passed |
| +1 :green_heart: | compile | 1m 13s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | compile | 1m 2s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | checkstyle | 0m 54s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 7s | | trunk passed |
| +1 :green_heart: | javadoc | 0m 56s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 0m 51s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 2m 7s | | trunk passed |
| +1 :green_heart: | shadedclient | 24m 35s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| -1 :x: | mvninstall | 0m 53s | [/patch-mvninstall-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/artifact/out/patch-mvninstall-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt) | hadoop-yarn-server-resourcemanager in the patch failed. |
| -1 :x: | compile | 0m 59s | [/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkUbuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/artifact/out/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkUbuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04.txt) | hadoop-yarn-server-resourcemanager in the patch failed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04. |
| -1 :x: | javac | 0m 59s | [/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkUbuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/artifact/out/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkUbuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04.txt) | hadoop-yarn-server-resourcemanager in the patch failed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04. |
| -1 :x: | compile | 0m 50s | [/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkPrivateBuild-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/artifact/out/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkPrivateBuild-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10.txt) | hadoop-yarn-server-resourcemanager in the patch failed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10. |
| -1 :x: | javac | 0m 50s | [/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkPrivateBuild-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/artifact/out/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdkPrivateBuild-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10.txt) | hadoop-yarn-server-resourcemanager in the patch failed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10. |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 0m 41s | [/results-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/artifact/out/results-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt) | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 1 new + 367 unchanged - 8 fixed = 368 total (was 375) |
| -1 :x: | mvnsite | 0m 55s | [/patch-mvnsite-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/artifact/out/patch-mvnsite-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt) | hadoop-yarn-server-resourcemanager in the patch failed. |
| +1 :green_heart: | javadoc | 0m 43s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 0m 39s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| -1 :x: | spotbugs | 0m 49s | [/patch-spotbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/artifact/out/patch-spotbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt) | hadoop-yarn-server-resourcemanager in the patch failed. |
| -1 :x: | shadedclient | 14m 19s | | patch has errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| -1 :x: | unit | 0m 57s | [/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt) | hadoop-yarn-server-resourcemanager in the patch failed. |
| +1 :green_heart: | asflicense | 0m 32s | | The patch does not generate ASF License warnings. |
| | | 86m 4s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/3660 |
| JIRA Issue | YARN-10982 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux 28dc9d5677a4 4.15.0-156-generic #163-Ubuntu SMP Thu Aug 19 23:31:58 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / fd28a711e568ae463ab63ba20a9921b6c7805f9b |
| Default Java | Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/testReport/ |
| Max. process+thread count | 711 (vs. ulimit of 5500) |
| modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/3/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
--
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
[GitHub] [hadoop] szilard-nemeth commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
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: 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
[GitHub] [hadoop] 9uapaw commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
9uapaw commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765131244
##########
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:
Agreed, let's handle it in a followup!
--
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
[GitHub] [hadoop] brumi1024 commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
brumi1024 commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765110750
##########
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:
+1, even the QueuePath object could have some kind of support for this.
--
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
[GitHub] [hadoop] TiborKovacsCloudera commented on a change in pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
TiborKovacsCloudera commented on a change in pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#discussion_r765487582
##########
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 is used at here: TestRMWebServicesCapacitySched@verifySubQueueXML at line 339
for (int k = 0; k < qListInfos.getLength(); k++) {
Element qElem3 = (Element) qListInfos.item(k);
String qName3 = WebServicesTestUtils.getXmlString(qElem3, "queueName");
**String q3 = QueuePath.concatenatePath(queue, qName3);**
verifySubQueueXML(qElem3, q3, qi.absoluteCapacity, qi.absoluteMaxCapacity);
}
--
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
[GitHub] [hadoop] hadoop-yetus commented on pull request #3660: YARN-10982: Replace all occurences of queuePath with the new QueuePath class
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on pull request #3660:
URL: https://github.com/apache/hadoop/pull/3660#issuecomment-989929479
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 37s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 9 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 32m 18s | | trunk passed |
| +1 :green_heart: | compile | 1m 5s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | compile | 0m 56s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | checkstyle | 0m 51s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 1s | | trunk passed |
| +1 :green_heart: | javadoc | 0m 50s | | trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 0m 45s | | trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 1m 55s | | trunk passed |
| +1 :green_heart: | shadedclient | 21m 8s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 0m 52s | | the patch passed |
| +1 :green_heart: | compile | 0m 56s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javac | 0m 56s | | the patch passed |
| +1 :green_heart: | compile | 0m 49s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | javac | 0m 49s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 40s | | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 341 unchanged - 8 fixed = 341 total (was 349) |
| +1 :green_heart: | mvnsite | 0m 51s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 42s | | the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 |
| +1 :green_heart: | javadoc | 0m 37s | | the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| +1 :green_heart: | spotbugs | 1m 57s | | the patch passed |
| +1 :green_heart: | shadedclient | 20m 39s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 96m 13s | | hadoop-yarn-server-resourcemanager in the patch passed. |
| +1 :green_heart: | asflicense | 0m 34s | | The patch does not generate ASF License warnings. |
| | | 185m 37s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/6/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/3660 |
| JIRA Issue | YARN-10982 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux 6af7de9170f1 4.15.0-156-generic #163-Ubuntu SMP Thu Aug 19 23:31:58 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 39f4ec203377244f840e4593aa02386ff51cc3c4 |
| Default Java | Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/6/testReport/ |
| Max. process+thread count | 965 (vs. ulimit of 5500) |
| modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3660/6/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
--
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