YARN-10917. Investigate and simplify CapacitySchedulerConfigValidator#validateQueueHierarchy (#3403)

* YARN-10917. Investigate and simplify CapacitySchedulerConfigValidator#validateQueueHierarchy.

Co-authored-by: Tamas Domok <tdomok@cloudera.com>

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfigValidator.java

@@ -106,12 +106,8 @@ public static void validateVCores(Configuration conf) {
     }
   }
 
-  private static boolean isDynamicQueue(CSQueue csQueue) {
-    return ((AbstractCSQueue)csQueue).isDynamicQueue();
-  }
-
   /**
-   * Ensure all existing queues are present. Queues cannot be deleted if its not
+   * Ensure all existing queues are present. Queues cannot be deleted if it's not
    * in Stopped state, Queue's cannot be moved from one hierarchy to other also.
    * Previous child queue could be converted into parent queue if it is in
    * STOPPED state.
@@ -125,28 +121,22 @@ public static void validateQueueHierarchy(
       CapacitySchedulerConfiguration newConf) throws IOException {
     // check that all static queues are included in the newQueues list
     for (CSQueue oldQueue : queues.getQueues()) {
-      if (!(AbstractAutoCreatedLeafQueue.class.isAssignableFrom(
+      if (AbstractAutoCreatedLeafQueue.class.isAssignableFrom(oldQueue.getClass())) {
-          oldQueue.getClass()))) {
+        continue;
-        String queuePath = oldQueue.getQueuePath();
+      }
-        CSQueue newQueue = newQueues.get(queuePath);
+
-        String configPrefix = newConf.getQueuePrefix(
+      final String queuePath = oldQueue.getQueuePath();
+      final String configPrefix = CapacitySchedulerConfiguration.getQueuePrefix(
           oldQueue.getQueuePath());
-        String state = newConf.get(configPrefix + "state");
+      final QueueState newQueueState = createQueueState(newConf.get(configPrefix + "state"),
-        QueueState newQueueState = null;
+          queuePath);
-        if (state != null) {
+      final CSQueue newQueue = newQueues.get(queuePath);
-          try {
+
-            newQueueState = QueueState.valueOf(state);
-          } catch (Exception ex) {
-            LOG.warn("Not a valid queue state for queue "
-                + oldQueue.getQueuePath());
-          }
-        }
       if (null == newQueue) {
         // old queue doesn't exist in the new XML
-          if (oldQueue.getState() == QueueState.STOPPED ||
+        if (isEitherQueueStopped(oldQueue.getState(), newQueueState)) {
-              newQueueState == QueueState.STOPPED) {
+          LOG.info("Deleting Queue {}, as it is not present in the modified capacity " +
-            LOG.info("Deleting Queue " + queuePath + ", as it is not"
+              "configuration xml", queuePath);
-                + " present in the modified capacity configuration xml");
         } else {
           if (!isDynamicQueue(oldQueue)) {
             throw new IOException(oldQueue.getQueuePath() + " cannot be"
@@ -155,48 +145,82 @@ public static void validateQueueHierarchy(
                 + oldQueue.getState());
           }
         }
-        } else if (!oldQueue.getQueuePath().equals(newQueue.getQueuePath())) {
+      } else {
-          //Queue's cannot be moved from one hierarchy to other
+        validateSameQueuePath(oldQueue, newQueue);
+        validateParentQueueConversion(oldQueue, newQueue);
+        validateLeafQueueConversion(oldQueue, newQueue);
+      }
+    }
+  }
+
+  private static void validateSameQueuePath(CSQueue oldQueue, CSQueue newQueue) throws IOException {
+    if (!oldQueue.getQueuePath().equals(newQueue.getQueuePath())) {
+      // Queues cannot be moved from one hierarchy to another
       throw new IOException(
-            queuePath + " is moved from:" + oldQueue.getQueuePath() + " to:"
+          oldQueue.getQueuePath() + " is moved from:" + oldQueue.getQueuePath() + " to:"
               + newQueue.getQueuePath()
               + " after refresh, which is not allowed.");
-        } else if (oldQueue instanceof ParentQueue
+    }
-            && !(oldQueue instanceof ManagedParentQueue)
+  }
-            && newQueue instanceof ManagedParentQueue) {
+
+  private static void validateParentQueueConversion(CSQueue oldQueue,
+                                                    CSQueue newQueue) throws IOException {
+    if (oldQueue instanceof ParentQueue) {
+      if (!(oldQueue instanceof ManagedParentQueue) && newQueue instanceof ManagedParentQueue) {
         throw new IOException(
             "Can not convert parent queue: " + oldQueue.getQueuePath()
                 + " to auto create enabled parent queue since "
                 + "it could have other pre-configured queues which is not "
                 + "supported");
-        } else if (oldQueue instanceof ManagedParentQueue
+      }
+
+      if (oldQueue instanceof ManagedParentQueue
           && !(newQueue instanceof ManagedParentQueue)) {
         throw new IOException(
             "Cannot convert auto create enabled parent queue: "
                 + oldQueue.getQueuePath() + " to leaf queue. Please check "
                 + " parent queue's configuration "
-              + CapacitySchedulerConfiguration
+                + CapacitySchedulerConfiguration.AUTO_CREATE_CHILD_QUEUE_ENABLED
-              .AUTO_CREATE_CHILD_QUEUE_ENABLED
                 + " is set to true");
-        } else if (oldQueue instanceof LeafQueue
+      }
-                && newQueue instanceof ParentQueue) {
+
-          if (oldQueue.getState() == QueueState.STOPPED ||
+      if (newQueue instanceof LeafQueue) {
-              newQueueState == QueueState.STOPPED) {
+        LOG.info("Converting the parent queue: {} to leaf queue.", oldQueue.getQueuePath());
-            LOG.info("Converting the leaf queue: " + oldQueue.getQueuePath()
+      }
-                + " to parent queue.");
+    }
-          } else{
+  }
+
+  private static void validateLeafQueueConversion(CSQueue oldQueue,
+                                                  CSQueue newQueue) throws IOException {
+    if (oldQueue instanceof LeafQueue && newQueue instanceof ParentQueue) {
+      if (isEitherQueueStopped(oldQueue.getState(), newQueue.getState())) {
+        LOG.info("Converting the leaf queue: {} to parent queue.", oldQueue.getQueuePath());
+      } else {
         throw new IOException(
             "Can not convert the leaf queue: " + oldQueue.getQueuePath()
                 + " to parent queue since "
                 + "it is not yet in stopped state. Current State : "
                 + oldQueue.getState());
       }
-        } else if (oldQueue instanceof ParentQueue
-            && newQueue instanceof LeafQueue) {
-          LOG.info("Converting the parent queue: " + oldQueue.getQueuePath()
-              + " to leaf queue.");
     }
   }
+
+  private static QueueState createQueueState(String state, String queuePath) {
+    if (state != null) {
+      try {
+        return QueueState.valueOf(state);
+      } catch (Exception ex) {
+        LOG.warn("Not a valid queue state for queue: {}, state: {}", queuePath, state);
       }
     }
+    return null;
+  }
+
+  private static boolean isDynamicQueue(CSQueue csQueue) {
+    return ((AbstractCSQueue)csQueue).isDynamicQueue();
+  }
+
+  private static boolean isEitherQueueStopped(QueueState a, QueueState b) {
+    return a == QueueState.STOPPED || b == QueueState.STOPPED;
+  }
 }