From ec913398a9b6c75610c0ae3d6af4f41e11383d96 Mon Sep 17 00:00:00 2001 From: Szilard Nemeth Date: Wed, 17 Jun 2020 14:36:08 +0200 Subject: [PATCH] YARN-10281. Redundant QueuePath usage in UserGroupMappingPlacementRule and AppNameMappingPlacementRule. Contributed by Gergely Pollak --- .../AppNameMappingPlacementRule.java | 11 ++-- .../placement/QueueMapping.java | 24 +++++--- .../resourcemanager/placement/QueuePath.java | 61 ------------------- .../placement/QueuePlacementRuleUtils.java | 31 +++------- .../UserGroupMappingPlacementRule.java | 35 ++++++----- .../CapacitySchedulerConfiguration.java | 4 +- .../scheduler/capacity/TestQueueMappings.java | 27 ++++++++ 7 files changed, 73 insertions(+), 120 deletions(-) delete mode 100644 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueuePath.java diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/AppNameMappingPlacementRule.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/AppNameMappingPlacementRule.java index cf725b6286..63d98ba6c4 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/AppNameMappingPlacementRule.java +++ b/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, - queuePath); + validateAndGetAutoCreatedQueueMapping(queueManager, mapping); 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{ diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueueMapping.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueueMapping.java index 3fcb5fe6b8..b142dd6c10 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueueMapping.java +++ b/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) { - this.queue = path.getLeafQueue(); - this.parentQueue = path.getParentQueue(); - return this; + public QueueMappingBuilder parsePathString(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 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; diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueuePath.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueuePath.java deleted file mode 100644 index e02cf58145..0000000000 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueuePath.java +++ /dev/null @@ -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; - } -} diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueuePlacementRuleUtils.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueuePlacementRuleUtils.java index 350f2b93d8..15c8fd8b70 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/QueuePlacementRuleUtils.java +++ b/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, - QueuePath queuePath) throws IOException { - if (queuePath.hasParentQueue()) { + CapacitySchedulerQueueManager queueManager, QueueMapping mapping) + throws IOException { + 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(), - queuePath.getFullPath()); + mapping.getParentQueue()), mapping.getParentQueue(), + 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 { diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/UserGroupMappingPlacementRule.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/UserGroupMappingPlacementRule.java index 6281e52c66..dfa49bea89 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/UserGroupMappingPlacementRule.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/UserGroupMappingPlacementRule.java @@ -388,7 +388,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 @@ -409,7 +408,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 @@ -422,7 +421,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{ @@ -433,7 +432,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{ @@ -455,7 +454,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 : " + @@ -466,7 +465,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 " @@ -482,29 +481,29 @@ private static boolean ifQueueDoesNotExist(CSQueue queue) { } private static QueueMapping validateAndGetAutoCreatedQueueMapping( - CapacitySchedulerQueueManager queueManager, QueueMapping mapping, - QueuePath queuePath) throws IOException { - if (queuePath.hasParentQueue() - && (queuePath.getParentQueue().equals(PRIMARY_GROUP_MAPPING) - || queuePath.getParentQueue().equals(SECONDARY_GROUP_MAPPING))) { + CapacitySchedulerQueueManager queueManager, QueueMapping mapping) + throws IOException { + if (mapping.hasParentQueue() + && (mapping.getParentQueue().equals(PRIMARY_GROUP_MAPPING) + || mapping.getParentQueue().equals(SECONDARY_GROUP_MAPPING))) { // dynamic parent queue return QueueMappingBuilder.create() .type(mapping.getType()) .source(mapping.getSource()) - .queue(queuePath.getLeafQueue()) - .parentQueue(queuePath.getParentQueue()) + .queue(mapping.getQueue()) + .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()), - queuePath.getParentQueue(), queuePath.getLeafQueue()); + queueManager.getQueue(mapping.getParentQueue()), + mapping.getParentQueue(), mapping.getQueue()); return QueueMappingBuilder.create() .type(mapping.getType()) .source(mapping.getSource()) - .queue(queuePath.getLeafQueue()) - .parentQueue(queuePath.getParentQueue()) + .queue(mapping.getQueue()) + .parentQueue(mapping.getParentQueue()) .build(); } diff --git a/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 b/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 index d5a7c84cd9..3b678c41ad 100644 --- a/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 +++ b/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 @@ -1055,7 +1055,7 @@ public List 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); } @@ -1131,7 +1131,7 @@ public List getQueueMappings() { m = QueueMappingBuilder.create() .type(mappingType) .source(mapping[1]) - .queuePath(QueuePlacementRuleUtils.extractQueuePath(mapping[2])) + .parsePathString(mapping[2]) .build(); } catch (Throwable t) { throw new IllegalArgumentException( diff --git a/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 b/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 index 2e7009eae6..039b9da8aa 100644 --- a/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 +++ b/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