shuzirra commented on a change in pull request #3342:
URL: https://github.com/apache/hadoop/pull/3342#discussion_r712132340
##########
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
##########
@@ -116,8 +120,79 @@ public boolean hasParent() {
return parent != null;
}
+ /**
+ * Creates a new {@code QueuePath} from the current full path as parent, and
+ * the appended child queue path as leaf.
+ * @param childQueue path of leaf queue
+ * @return new queue path made of current full path and appended leaf path
+ */
+ public QueuePath createNewLeaf(String childQueue) {
+ return new QueuePath(getFullPath(), childQueue);
+ }
+
+ /**
+ * Returns an iterator of queue path parts, starting from the highest level
+ * (generally root).
+ * @return queue part iterator
+ */
+ @Override
+ public Iterator<String> iterator() {
+ return
Arrays.asList(getFullPath().split(QUEUE_REGEX_DELIMITER)).iterator();
+ }
+
+ /**
+ * Returns an iterator that provides a way to traverse the queue path from
+ * current queue through its parents.
+ * @return queue path iterator
+ */
+ public Iterator<QueuePath> reversePathIterator() {
+ QueuePath initial = this;
Review comment:
We don't need this as a filed, used only once, to set the initial
"pointer" in the iterator.
##########
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
##########
@@ -116,8 +120,79 @@ public boolean hasParent() {
return parent != null;
}
+ /**
+ * Creates a new {@code QueuePath} from the current full path as parent, and
+ * the appended child queue path as leaf.
+ * @param childQueue path of leaf queue
+ * @return new queue path made of current full path and appended leaf path
+ */
+ public QueuePath createNewLeaf(String childQueue) {
+ return new QueuePath(getFullPath(), childQueue);
+ }
+
+ /**
+ * Returns an iterator of queue path parts, starting from the highest level
+ * (generally root).
+ * @return queue part iterator
+ */
+ @Override
+ public Iterator<String> iterator() {
+ return
Arrays.asList(getFullPath().split(QUEUE_REGEX_DELIMITER)).iterator();
+ }
+
+ /**
+ * Returns an iterator that provides a way to traverse the queue path from
+ * current queue through its parents.
+ * @return queue path iterator
+ */
+ public Iterator<QueuePath> reversePathIterator() {
+ QueuePath initial = this;
+
+ return new Iterator<QueuePath>() {
+ private boolean stop = false;
+ private QueuePath current = initial;
+
+ @Override
+ public boolean hasNext() {
+ return !stop;
+ }
+
+ @Override
+ public QueuePath next() {
+ QueuePath old = current;
+ if (current.hasParent()) {
+ current = new QueuePath(current.getParent());
+ }
Review comment:
If you'd introduce an else branch here, and set current = null, then we
could use current == null as the stop indicator, and we wouldn't need the stop
variable at all.
##########
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
##########
@@ -116,8 +120,79 @@ public boolean hasParent() {
return parent != null;
}
+ /**
+ * Creates a new {@code QueuePath} from the current full path as parent, and
+ * the appended child queue path as leaf.
+ * @param childQueue path of leaf queue
+ * @return new queue path made of current full path and appended leaf path
+ */
+ public QueuePath createNewLeaf(String childQueue) {
+ return new QueuePath(getFullPath(), childQueue);
+ }
+
+ /**
+ * Returns an iterator of queue path parts, starting from the highest level
+ * (generally root).
+ * @return queue part iterator
+ */
+ @Override
+ public Iterator<String> iterator() {
+ return
Arrays.asList(getFullPath().split(QUEUE_REGEX_DELIMITER)).iterator();
+ }
+
+ /**
+ * Returns an iterator that provides a way to traverse the queue path from
+ * current queue through its parents.
+ * @return queue path iterator
+ */
+ public Iterator<QueuePath> reversePathIterator() {
+ QueuePath initial = this;
+
+ return new Iterator<QueuePath>() {
+ private boolean stop = false;
+ private QueuePath current = initial;
+
+ @Override
+ public boolean hasNext() {
+ return !stop;
+ }
+
+ @Override
+ public QueuePath next() {
+ QueuePath old = current;
Review comment:
please add a hasNext check here, and throw a NoSuchElementException if
we are out of parents. Currently we can make an infinite loop with this
iterator since it will always return the last element if we are at the end of
the list.
##########
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/QueuePathTest.java
##########
@@ -0,0 +1,106 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity;
+
+import org.apache.hadoop.thirdparty.com.google.common.collect.ImmutableList;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.List;
+
+public class QueuePathTest {
+ private static final String TEST_QUEUE = "root.level_1.level_2.level_3";
+
+ @Test
+ public void testCreation() {
+ QueuePath queuePath = new QueuePath(TEST_QUEUE);
+
+ Assert.assertEquals(TEST_QUEUE, queuePath.getFullPath());
+ Assert.assertEquals("root.level_1.level_2", queuePath.getParent());
+ Assert.assertEquals("level_3", queuePath.getLeafName());
+
+ QueuePath rootPath = new QueuePath(CapacitySchedulerConfiguration.ROOT);
+ Assert.assertNull(rootPath.getParent());
+
+ QueuePath appendedPath = queuePath.createNewLeaf("level_4");
+ Assert.assertEquals(TEST_QUEUE + CapacitySchedulerConfiguration.DOT
+ + "level_4", appendedPath.getFullPath());
+ Assert.assertEquals("root.level_1.level_2.level_3",
appendedPath.getParent());
+ Assert.assertEquals("level_4", appendedPath.getLeafName());
+ }
+
+ @Test
+ public void testEmptyPart() {
+ QueuePath queuePathWithEmptyPart = new QueuePath("root..level_2");
+ QueuePath queuePathWithoutEmptyPart = new QueuePath(TEST_QUEUE);
+
+ Assert.assertTrue(queuePathWithEmptyPart.hasEmptyPart());
+ Assert.assertFalse(queuePathWithoutEmptyPart.hasEmptyPart());
+ }
+
+ @Test
+ public void testIterator() {
+ QueuePath queuePath = new QueuePath(TEST_QUEUE);
+ QueuePath queuePathWithEmptyPart = new QueuePath("root..level_2");
+ QueuePath rootPath = new QueuePath(CapacitySchedulerConfiguration.ROOT);
+
+ List<String> queuePathCollection =
ImmutableList.copyOf(queuePath.iterator());
+ List<String> queuePathWithEmptyPartCollection = ImmutableList.copyOf(
+ queuePathWithEmptyPart.iterator());
+ List<String> rootPathCollection =
ImmutableList.copyOf(rootPath.iterator());
+
+ Assert.assertEquals(4, queuePathCollection.size());
+ Assert.assertEquals(CapacitySchedulerConfiguration.ROOT,
queuePathCollection.get(0));
+ Assert.assertEquals("level_3", queuePathCollection.get(3));
+
+ Assert.assertEquals(3, queuePathWithEmptyPartCollection.size());
+ Assert.assertEquals(CapacitySchedulerConfiguration.ROOT,
+ queuePathWithEmptyPartCollection.get(0));
+ Assert.assertEquals("level_2", queuePathWithEmptyPartCollection.get(2));
+
+ Assert.assertEquals(1, rootPathCollection.size());
+ Assert.assertEquals(CapacitySchedulerConfiguration.ROOT,
rootPathCollection.get(0));
+ }
+
+ @Test
+ public void testReversePathIterator() {
+ QueuePath queuePath = new QueuePath(TEST_QUEUE);
+ QueuePath queuePathWithEmptyPart = new QueuePath("root..level_2");
+ QueuePath rootPath = new QueuePath(CapacitySchedulerConfiguration.ROOT);
+
+ List<QueuePath> queuePathCollection =
ImmutableList.copyOf(queuePath.reversePathIterator());
+ List<QueuePath> queuePathWithEmptyPartCollection = ImmutableList.copyOf(
+ queuePathWithEmptyPart.reversePathIterator());
+ List<QueuePath> rootPathCollection =
ImmutableList.copyOf(rootPath.reversePathIterator());
+
+ Assert.assertEquals(4, queuePathCollection.size());
+ Assert.assertEquals(CapacitySchedulerConfiguration.ROOT,
+ queuePathCollection.get(3).getFullPath());
+ Assert.assertEquals(TEST_QUEUE, queuePathCollection.get(0).getFullPath());
+
+ Assert.assertEquals(3, queuePathWithEmptyPartCollection.size());
+ Assert.assertEquals(CapacitySchedulerConfiguration.ROOT,
+ queuePathWithEmptyPartCollection.get(2).getFullPath());
+ Assert.assertEquals("root..level_2",
queuePathWithEmptyPartCollection.get(0).getFullPath());
+
+ Assert.assertEquals(1, rootPathCollection.size());
+ Assert.assertEquals(CapacitySchedulerConfiguration.ROOT,
+ rootPathCollection.get(0).getFullPath());
+ }
+}
Review comment:
Please add test cases for equals as well, with the corner cases like
empty path and null path, and null as other 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/QueuePath.java
##########
@@ -116,8 +120,79 @@ public boolean hasParent() {
return parent != null;
}
+ /**
+ * Creates a new {@code QueuePath} from the current full path as parent, and
+ * the appended child queue path as leaf.
+ * @param childQueue path of leaf queue
+ * @return new queue path made of current full path and appended leaf path
+ */
+ public QueuePath createNewLeaf(String childQueue) {
+ return new QueuePath(getFullPath(), childQueue);
+ }
+
+ /**
+ * Returns an iterator of queue path parts, starting from the highest level
+ * (generally root).
+ * @return queue part iterator
+ */
+ @Override
+ public Iterator<String> iterator() {
+ return
Arrays.asList(getFullPath().split(QUEUE_REGEX_DELIMITER)).iterator();
+ }
+
+ /**
+ * Returns an iterator that provides a way to traverse the queue path from
+ * current queue through its parents.
+ * @return queue path iterator
+ */
+ public Iterator<QueuePath> reversePathIterator() {
+ QueuePath initial = this;
+
+ return new Iterator<QueuePath>() {
+ private boolean stop = false;
+ private QueuePath current = initial;
Review comment:
Just use QueuePath.this instead
##########
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
##########
@@ -116,8 +120,79 @@ public boolean hasParent() {
return parent != null;
}
+ /**
+ * Creates a new {@code QueuePath} from the current full path as parent, and
+ * the appended child queue path as leaf.
+ * @param childQueue path of leaf queue
+ * @return new queue path made of current full path and appended leaf path
+ */
+ public QueuePath createNewLeaf(String childQueue) {
+ return new QueuePath(getFullPath(), childQueue);
+ }
+
+ /**
+ * Returns an iterator of queue path parts, starting from the highest level
+ * (generally root).
+ * @return queue part iterator
+ */
+ @Override
+ public Iterator<String> iterator() {
+ return
Arrays.asList(getFullPath().split(QUEUE_REGEX_DELIMITER)).iterator();
+ }
+
+ /**
+ * Returns an iterator that provides a way to traverse the queue path from
+ * current queue through its parents.
+ * @return queue path iterator
+ */
+ public Iterator<QueuePath> reversePathIterator() {
+ QueuePath initial = this;
+
+ return new Iterator<QueuePath>() {
+ private boolean stop = false;
+ private QueuePath current = initial;
+
+ @Override
+ public boolean hasNext() {
+ return !stop;
+ }
+
+ @Override
+ public QueuePath next() {
+ QueuePath old = current;
+ if (current.hasParent()) {
+ current = new QueuePath(current.getParent());
+ }
+
+ if (!old.hasParent()) {
+ stop = true;
+ }
+
+ return old;
+ }
+ };
+ }
+
@Override
public String toString() {
return getFullPath();
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ QueuePath strings = (QueuePath) o;
Review comment:
Please consider using a bit more expressing name here, like otherPath or
something like that, 'strings' is not too informative especially when it is an
other QueuePath object and not a string collection :)
##########
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
##########
@@ -116,8 +120,79 @@ public boolean hasParent() {
return parent != null;
}
+ /**
+ * Creates a new {@code QueuePath} from the current full path as parent, and
+ * the appended child queue path as leaf.
+ * @param childQueue path of leaf queue
+ * @return new queue path made of current full path and appended leaf path
+ */
+ public QueuePath createNewLeaf(String childQueue) {
+ return new QueuePath(getFullPath(), childQueue);
+ }
+
+ /**
+ * Returns an iterator of queue path parts, starting from the highest level
+ * (generally root).
+ * @return queue part iterator
+ */
+ @Override
+ public Iterator<String> iterator() {
+ return
Arrays.asList(getFullPath().split(QUEUE_REGEX_DELIMITER)).iterator();
+ }
+
+ /**
+ * Returns an iterator that provides a way to traverse the queue path from
+ * current queue through its parents.
+ * @return queue path iterator
+ */
+ public Iterator<QueuePath> reversePathIterator() {
Review comment:
I don't really like that the reverse path iterator iterates through
QueuePath objects, it seems a bit overkill to me and also inconsistent with the
regular iterator which iterates through the path parts (String).
How are we going to use this, do we really need this many queue QueuePath
objects? Wouldn't it be enough if we'd only return path parts just like the
simple iterator?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
