YARN-10281. Redundant QueuePath usage in UserGroupMappingPlacementRule and AppNameMappingPlacementRule. Contributed by Gergely Pollak

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/AppNameMappingPlacementRule.java

@@ -86,8 +86,6 @@ public boolean initialize(ResourceScheduler scheduler)
 
     // check if mappings refer to valid queues
     for (QueueMapping mapping : queueMappings) {
-      QueuePath queuePath = mapping.getQueuePath();
-
       if (isStaticQueueMapping(mapping)) {
         //at this point mapping.getQueueName() return only the queue name, since
         //the config parsing have been changed making QueueMapping more
@@ -98,7 +96,7 @@ public boolean initialize(ResourceScheduler scheduler)
           //Try getting queue by its full path name, if it exists it is a static
           //leaf queue indeed, without any auto creation magic
 
-          if (queueManager.isAmbiguous(queuePath.getFullPath())) {
+          if (queueManager.isAmbiguous(mapping.getFullPath())) {
             throw new IOException(
               "mapping contains ambiguous leaf queue reference " + mapping
                 .getFullPath());
@@ -110,8 +108,7 @@ public boolean initialize(ResourceScheduler scheduler)
           // then it should exist and
           // be an instance of AutoCreateEnabledParentQueue
           QueueMapping newMapping =
-              validateAndGetAutoCreatedQueueMapping(queueManager, mapping,
+              validateAndGetAutoCreatedQueueMapping(queueManager, mapping);
-                  queuePath);
           if (newMapping == null) {
             throw new IOException(
                 "mapping contains invalid or non-leaf queue " + mapping
@@ -124,7 +121,7 @@ public boolean initialize(ResourceScheduler scheduler)
           //   if its an instance of auto created leaf queue,
           // then extract parent queue name and update queue mapping
           QueueMapping newMapping = validateAndGetQueueMapping(
-              queueManager, queue, mapping, queuePath);
+              queueManager, queue, mapping);
           newMappings.add(newMapping);
         }
       } else {
@@ -135,7 +132,7 @@ public boolean initialize(ResourceScheduler scheduler)
         //  parent queue exists and an instance of AutoCreateEnabledParentQueue
         //
         QueueMapping newMapping = validateAndGetAutoCreatedQueueMapping(
-            queueManager, mapping, queuePath);
+            queueManager, mapping);
         if (newMapping != null) {
           newMappings.add(newMapping);
         } else{

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueueMapping.java

@@ -66,10 +66,20 @@ public QueueMappingBuilder parentQueue(String mappingParentQueue) {
       return this;
     }
 
-    public QueueMappingBuilder queuePath(QueuePath path) {
+    public QueueMappingBuilder parsePathString(String queuePath) {
-      this.queue = path.getLeafQueue();
+      int parentQueueNameEndIndex = queuePath.lastIndexOf(DOT);
-      this.parentQueue = path.getParentQueue();
+
-      return this;
+      if (parentQueueNameEndIndex > -1) {
+        final String parentQueue =
+            queuePath.substring(0, parentQueueNameEndIndex).trim();
+        final String leafQueue =
+            queuePath.substring(parentQueueNameEndIndex + 1).trim();
+        return this
+            .parentQueue(parentQueue)
+            .queue(leafQueue);
+      }
+
+      return this.queue(queuePath);
     }
 
     public QueueMapping build() {
@@ -138,12 +148,6 @@ public String getFullPath() {
     return fullPath;
   }
 
-  public QueuePath getQueuePath() {
-    //This is to make sure the parsing is the same everywhere, but the
-    //whole parsing part should be moved to QueuePathConstructor
-    return QueuePlacementRuleUtils.extractQueuePath(getFullPath());
-  }
-
   @Override
   public int hashCode() {
     final int prime = 31;

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueuePath.java

@@ -1,61 +0,0 @@
-/**
- * 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.placement;
-
-import static org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.DOT;
-
-public class QueuePath {
-
-  private String parentQueue;
-  private String leafQueue;
-  private String fullPath;
-
-  public QueuePath(final String leafQueue) {
-    //if the queue does not have a parent, the full path == leaf queue name
-    this.leafQueue = leafQueue;
-    this.fullPath  = leafQueue;
-  }
-
-  public QueuePath(final String parentQueue, final String leafQueue) {
-    this.parentQueue = parentQueue;
-    this.leafQueue = leafQueue;
-    this.fullPath = parentQueue + DOT + leafQueue;
-  }
-
-  public String getParentQueue() {
-    return parentQueue;
-  }
-
-  public String getLeafQueue() {
-    return leafQueue;
-  }
-
-  public boolean hasParentQueue() {
-    return parentQueue != null;
-  }
-
-  public String getFullPath() {
-    return fullPath;
-  }
-
-  @Override
-  public String toString() {
-    return fullPath;
-  }
-}

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueuePlacementRuleUtils.java

@@ -66,18 +66,19 @@ public static void validateQueueMappingUnderParentQueue(
   }
 
   public static QueueMapping validateAndGetAutoCreatedQueueMapping(
-      CapacitySchedulerQueueManager queueManager, QueueMapping mapping,
+      CapacitySchedulerQueueManager queueManager, QueueMapping mapping)
-      QueuePath queuePath) throws IOException {
+      throws IOException {
-    if (queuePath.hasParentQueue()) {
+    if (mapping.hasParentQueue()) {
       //if parent queue is specified,
       // then it should exist and be an instance of ManagedParentQueue
       validateQueueMappingUnderParentQueue(queueManager.getQueue(
-          queuePath.getParentQueue()), queuePath.getParentQueue(),
+          mapping.getParentQueue()), mapping.getParentQueue(),
-          queuePath.getFullPath());
+          mapping.getFullPath());
       return QueueMapping.QueueMappingBuilder.create()
           .type(mapping.getType())
           .source(mapping.getSource())
-          .queuePath(queuePath)
+          .parentQueue(mapping.getParentQueue())
+          .queue(mapping.getQueue())
           .build();
     }
 
@@ -86,7 +87,7 @@ public static QueueMapping validateAndGetAutoCreatedQueueMapping(
 
   public static QueueMapping validateAndGetQueueMapping(
       CapacitySchedulerQueueManager queueManager, CSQueue queue,
-      QueueMapping mapping, QueuePath queuePath) throws IOException {
+      QueueMapping mapping) throws IOException {
     if (!(queue instanceof LeafQueue)) {
       throw new IOException(
           "mapping contains invalid or non-leaf queue : " +
@@ -97,7 +98,7 @@ public static QueueMapping validateAndGetQueueMapping(
         .getParent() instanceof ManagedParentQueue) {
 
       QueueMapping newMapping = validateAndGetAutoCreatedQueueMapping(
-          queueManager, mapping, queuePath);
+          queueManager, mapping);
       if (newMapping == null) {
         throw new IOException(
             "mapping contains invalid or non-leaf queue " +
@@ -114,20 +115,6 @@ public static boolean isStaticQueueMapping(QueueMapping mapping) {
         && !mapping.getQueue().contains(SECONDARY_GROUP_MAPPING);
   }
 
-  public static QueuePath extractQueuePath(String queuePath) {
-    int parentQueueNameEndIndex = queuePath.lastIndexOf(DOT);
-
-    if (parentQueueNameEndIndex > -1) {
-      final String parentQueue = queuePath.substring(0, parentQueueNameEndIndex)
-          .trim();
-      final String leafQueue = queuePath.substring(parentQueueNameEndIndex + 1)
-          .trim();
-      return new QueuePath(parentQueue, leafQueue);
-    }
-
-    return new QueuePath(queuePath);
-  }
-
   public static ApplicationPlacementContext getPlacementContext(
       QueueMapping mapping, CapacitySchedulerQueueManager queueManager)
       throws IOException {

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/UserGroupMappingPlacementRule.java

@@ -386,7 +386,6 @@ public boolean initialize(ResourceScheduler scheduler)
       //at this point mapping.getQueueName() return only the queue name, since
       //the config parsing have been changed making QueueMapping more consistent
 
-      QueuePath queuePath = mapping.getQueuePath();
       if (isStaticQueueMapping(mapping)) {
         //Try getting queue by its full path name, if it exists it is a static
         //leaf queue indeed, without any auto creation magic
@@ -407,7 +406,7 @@ public boolean initialize(ResourceScheduler scheduler)
           // then it should exist and
           // be an instance of AutoCreateEnabledParentQueue
           QueueMapping newMapping = validateAndGetAutoCreatedQueueMapping(
-              queueManager, mapping, queuePath);
+              queueManager, mapping);
           if (newMapping == null) {
             throw new IOException(
                 "mapping contains invalid or non-leaf queue " + mapping
@@ -420,7 +419,7 @@ public boolean initialize(ResourceScheduler scheduler)
           //   if its an instance of auto created leaf queue,
           // then extract parent queue name and update queue mapping
           QueueMapping newMapping = validateAndGetQueueMapping(queueManager,
-              queue, mapping, queuePath);
+              queue, mapping);
           newMappings.add(newMapping);
         }
       } else{
@@ -431,7 +430,7 @@ public boolean initialize(ResourceScheduler scheduler)
         //  parent queue exists and an instance of AutoCreateEnabledParentQueue
         //
         QueueMapping newMapping = validateAndGetAutoCreatedQueueMapping(
-            queueManager, mapping, queuePath);
+            queueManager, mapping);
         if (newMapping != null) {
           newMappings.add(newMapping);
         } else{
@@ -453,7 +452,7 @@ public boolean initialize(ResourceScheduler scheduler)
 
   private static QueueMapping validateAndGetQueueMapping(
       CapacitySchedulerQueueManager queueManager, CSQueue queue,
-      QueueMapping mapping, QueuePath queuePath) throws IOException {
+      QueueMapping mapping) throws IOException {
     if (!(queue instanceof LeafQueue)) {
       throw new IOException(
           "mapping contains invalid or non-leaf queue : " +
@@ -464,7 +463,7 @@ private static QueueMapping validateAndGetQueueMapping(
         .getParent() instanceof ManagedParentQueue) {
 
       QueueMapping newMapping = validateAndGetAutoCreatedQueueMapping(
-          queueManager, mapping, queuePath);
+          queueManager, mapping);
       if (newMapping == null) {
         throw new IOException(
             "mapping contains invalid or non-leaf queue "
@@ -480,29 +479,29 @@ private static boolean ifQueueDoesNotExist(CSQueue queue) {
   }
 
   private static QueueMapping validateAndGetAutoCreatedQueueMapping(
-      CapacitySchedulerQueueManager queueManager, QueueMapping mapping,
+      CapacitySchedulerQueueManager queueManager, QueueMapping mapping)
-      QueuePath queuePath) throws IOException {
+      throws IOException {
-    if (queuePath.hasParentQueue()
+    if (mapping.hasParentQueue()
-        && (queuePath.getParentQueue().equals(PRIMARY_GROUP_MAPPING)
+        && (mapping.getParentQueue().equals(PRIMARY_GROUP_MAPPING)
-            || queuePath.getParentQueue().equals(SECONDARY_GROUP_MAPPING))) {
+            || mapping.getParentQueue().equals(SECONDARY_GROUP_MAPPING))) {
       // dynamic parent queue
       return QueueMappingBuilder.create()
           .type(mapping.getType())
           .source(mapping.getSource())
-          .queue(queuePath.getLeafQueue())
+          .queue(mapping.getQueue())
-          .parentQueue(queuePath.getParentQueue())
+          .parentQueue(mapping.getParentQueue())
           .build();
-    } else if (queuePath.hasParentQueue()) {
+    } else if (mapping.hasParentQueue()) {
       //if parent queue is specified,
       // then it should exist and be an instance of ManagedParentQueue
       QueuePlacementRuleUtils.validateQueueMappingUnderParentQueue(
-              queueManager.getQueue(queuePath.getParentQueue()),
+              queueManager.getQueue(mapping.getParentQueue()),
-          queuePath.getParentQueue(), queuePath.getLeafQueue());
+          mapping.getParentQueue(), mapping.getQueue());
       return QueueMappingBuilder.create()
           .type(mapping.getType())
           .source(mapping.getSource())
-          .queue(queuePath.getLeafQueue())
+          .queue(mapping.getQueue())
-          .parentQueue(queuePath.getParentQueue())
+          .parentQueue(mapping.getParentQueue())
           .build();
     }
 

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

@@ -1060,7 +1060,7 @@ public List<QueueMapping> getQueueMappingEntity(
       QueueMapping m = QueueMapping.QueueMappingBuilder.create()
           .type(QueueMapping.MappingType.APPLICATION)
           .source(mapping[0])
-          .queuePath(QueuePlacementRuleUtils.extractQueuePath(mapping[1]))
+          .parsePathString(mapping[1])
           .build();
       mappings.add(m);
     }
@@ -1136,7 +1136,7 @@ public List<QueueMapping> getQueueMappings() {
         m = QueueMappingBuilder.create()
                 .type(mappingType)
                 .source(mapping[1])
-                .queuePath(QueuePlacementRuleUtils.extractQueuePath(mapping[2]))
+                .parsePathString(mapping[2])
                 .build();
       } catch (Throwable t) {
         throw new IllegalArgumentException(

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestQueueMappings.java

@@ -100,6 +100,33 @@ public void testQueueMappingTrimSpaces() throws IOException {
                 .build());
   }
 
+  @Test
+  public void testQueueMappingPathParsing() {
+    QueueMapping leafOnly = QueueMapping.QueueMappingBuilder.create()
+        .parsePathString("leaf")
+        .build();
+
+    Assert.assertEquals("leaf", leafOnly.getQueue());
+    Assert.assertEquals(null, leafOnly.getParentQueue());
+    Assert.assertEquals("leaf", leafOnly.getFullPath());
+
+    QueueMapping twoLevels = QueueMapping.QueueMappingBuilder.create()
+        .parsePathString("root.leaf")
+        .build();
+
+    Assert.assertEquals("leaf", twoLevels.getQueue());
+    Assert.assertEquals("root", twoLevels.getParentQueue());
+    Assert.assertEquals("root.leaf", twoLevels.getFullPath());
+
+    QueueMapping deep = QueueMapping.QueueMappingBuilder.create()
+        .parsePathString("root.a.b.c.d.e.leaf")
+        .build();
+
+    Assert.assertEquals("leaf", deep.getQueue());
+    Assert.assertEquals("root.a.b.c.d.e", deep.getParentQueue());
+    Assert.assertEquals("root.a.b.c.d.e.leaf", deep.getFullPath());
+  }
+
   @Test (timeout = 60000)
   public void testQueueMappingParsingInvalidCases() throws Exception {
     // configuration parsing tests - negative test cases