9uapaw commented on a change in pull request #3403:
URL: https://github.com/apache/hadoop/pull/3403#discussion_r705458694
##########
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/CapacitySchedulerConfigValidator.java
##########
@@ -125,76 +133,107 @@ 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(
- oldQueue.getClass()))) {
- String queuePath = oldQueue.getQueuePath();
- CSQueue newQueue = newQueues.get(queuePath);
- String configPrefix = newConf.getQueuePrefix(
- oldQueue.getQueuePath());
- String state = newConf.get(configPrefix + "state");
- QueueState newQueueState = null;
- if (state != null) {
- try {
- newQueueState = QueueState.valueOf(state);
- } catch (Exception ex) {
- LOG.warn("Not a valid queue state for queue "
- + oldQueue.getQueuePath());
+ if
(AbstractAutoCreatedLeafQueue.class.isAssignableFrom(oldQueue.getClass())) {
+ continue;
+ }
+
+ final String queuePath = oldQueue.getQueuePath();
+ final String configPrefix =
CapacitySchedulerConfiguration.getQueuePrefix(
+ oldQueue.getQueuePath());
+ final QueueState newQueueState = getQueueState(oldQueue,
newConf.get(configPrefix + "state"));
+ final CSQueue newQueue = newQueues.get(queuePath);
+
+ if (null == newQueue) {
+ // old queue doesn't exist in the new XML
+ if (oldQueue.getState() == QueueState.STOPPED || newQueueState ==
QueueState.STOPPED) {
+ LOG.info("Deleting Queue {}, as it is not present in the modified
capacity " +
+ "configuration xml", queuePath);
+ } else {
+ if (!isDynamicQueue(oldQueue)) {
+ throw new IOException(oldQueue.getQueuePath() + " cannot be"
+ + " deleted from the capacity scheduler configuration, as the"
+ + " queue is not yet in stopped state. Current State : "
+ + oldQueue.getState());
}
}
- if (null == newQueue) {
- // old queue doesn't exist in the new XML
- if (oldQueue.getState() == QueueState.STOPPED ||
- newQueueState == QueueState.STOPPED) {
- LOG.info("Deleting Queue " + queuePath + ", as it is not"
- + " present in the modified capacity configuration xml");
- } else {
- if (!isDynamicQueue(oldQueue)) {
- throw new IOException(oldQueue.getQueuePath() + " cannot be"
- + " deleted from the capacity scheduler configuration, as
the"
- + " queue is not yet in stopped state. Current State : "
- + oldQueue.getState());
- }
- }
- } else if (!oldQueue.getQueuePath().equals(newQueue.getQueuePath())) {
- //Queue's cannot be moved from one hierarchy to other
- throw new IOException(
- queuePath + " 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) {
+ } else {
+ for (QueueChangeValidator validator : QUEUE_CHANGE_VALIDATORS) {
+ validator.validate(oldQueue, newQueue);
+ }
+ }
+ }
+ }
+
+ private static QueueState getQueueState(CSQueue queue, String state) {
Review comment:
This method is a bit misleading. It is creating a new state from a
string. Also it does not need the full queue as its argument, only the path.
##########
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/CapacitySchedulerConfigValidator.java
##########
@@ -125,76 +133,107 @@ 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(
- oldQueue.getClass()))) {
- String queuePath = oldQueue.getQueuePath();
- CSQueue newQueue = newQueues.get(queuePath);
- String configPrefix = newConf.getQueuePrefix(
- oldQueue.getQueuePath());
- String state = newConf.get(configPrefix + "state");
- QueueState newQueueState = null;
- if (state != null) {
- try {
- newQueueState = QueueState.valueOf(state);
- } catch (Exception ex) {
- LOG.warn("Not a valid queue state for queue "
- + oldQueue.getQueuePath());
+ if
(AbstractAutoCreatedLeafQueue.class.isAssignableFrom(oldQueue.getClass())) {
+ continue;
+ }
+
+ final String queuePath = oldQueue.getQueuePath();
+ final String configPrefix =
CapacitySchedulerConfiguration.getQueuePrefix(
+ oldQueue.getQueuePath());
+ final QueueState newQueueState = getQueueState(oldQueue,
newConf.get(configPrefix + "state"));
+ final CSQueue newQueue = newQueues.get(queuePath);
+
+ if (null == newQueue) {
+ // old queue doesn't exist in the new XML
+ if (oldQueue.getState() == QueueState.STOPPED || newQueueState ==
QueueState.STOPPED) {
+ LOG.info("Deleting Queue {}, as it is not present in the modified
capacity " +
+ "configuration xml", queuePath);
+ } else {
+ if (!isDynamicQueue(oldQueue)) {
+ throw new IOException(oldQueue.getQueuePath() + " cannot be"
+ + " deleted from the capacity scheduler configuration, as the"
+ + " queue is not yet in stopped state. Current State : "
+ + oldQueue.getState());
}
}
- if (null == newQueue) {
- // old queue doesn't exist in the new XML
- if (oldQueue.getState() == QueueState.STOPPED ||
- newQueueState == QueueState.STOPPED) {
- LOG.info("Deleting Queue " + queuePath + ", as it is not"
- + " present in the modified capacity configuration xml");
- } else {
- if (!isDynamicQueue(oldQueue)) {
- throw new IOException(oldQueue.getQueuePath() + " cannot be"
- + " deleted from the capacity scheduler configuration, as
the"
- + " queue is not yet in stopped state. Current State : "
- + oldQueue.getState());
- }
- }
- } else if (!oldQueue.getQueuePath().equals(newQueue.getQueuePath())) {
- //Queue's cannot be moved from one hierarchy to other
- throw new IOException(
- queuePath + " 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) {
+ } else {
+ for (QueueChangeValidator validator : QUEUE_CHANGE_VALIDATORS) {
+ validator.validate(oldQueue, newQueue);
+ }
+ }
+ }
+ }
+
+ private static QueueState getQueueState(CSQueue queue, String state) {
+ if (state != null) {
+ try {
+ return QueueState.valueOf(state);
+ } catch (Exception ex) {
+ LOG.warn("Not a valid queue state for queue: {}, state: {}",
queue.getQueuePath(), state);
+ }
+ }
+ return null;
+ }
+
+ private static boolean isDynamicQueue(CSQueue csQueue) {
+ return ((AbstractCSQueue)csQueue).isDynamicQueue();
+ }
+
+ private static class SamePathValidator implements QueueChangeValidator {
+ @Override
+ public void validate(CSQueue oldQueue, CSQueue newQueue) throws
IOException {
+ if (!oldQueue.getQueuePath().equals(newQueue.getQueuePath())) {
+ // Queue's cannot be moved from one hierarchy to other
Review comment:
Nit: its a typo.
##########
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/CapacitySchedulerConfigValidator.java
##########
@@ -125,76 +133,107 @@ 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(
- oldQueue.getClass()))) {
- String queuePath = oldQueue.getQueuePath();
- CSQueue newQueue = newQueues.get(queuePath);
- String configPrefix = newConf.getQueuePrefix(
- oldQueue.getQueuePath());
- String state = newConf.get(configPrefix + "state");
- QueueState newQueueState = null;
- if (state != null) {
- try {
- newQueueState = QueueState.valueOf(state);
- } catch (Exception ex) {
- LOG.warn("Not a valid queue state for queue "
- + oldQueue.getQueuePath());
+ if
(AbstractAutoCreatedLeafQueue.class.isAssignableFrom(oldQueue.getClass())) {
+ continue;
+ }
+
+ final String queuePath = oldQueue.getQueuePath();
+ final String configPrefix =
CapacitySchedulerConfiguration.getQueuePrefix(
+ oldQueue.getQueuePath());
+ final QueueState newQueueState = getQueueState(oldQueue,
newConf.get(configPrefix + "state"));
+ final CSQueue newQueue = newQueues.get(queuePath);
+
+ if (null == newQueue) {
+ // old queue doesn't exist in the new XML
+ if (oldQueue.getState() == QueueState.STOPPED || newQueueState ==
QueueState.STOPPED) {
Review comment:
This idiom is repeated. It could be extracted as isEitherQueueStopped or
something similar.
##########
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/CapacitySchedulerConfigValidator.java
##########
@@ -27,14 +27,26 @@
import org.slf4j.LoggerFactory;
import java.io.IOException;
+import java.util.Arrays;
import java.util.Collection;
import java.util.LinkedHashSet;
+import java.util.List;
import java.util.Set;
public final class CapacitySchedulerConfigValidator {
private static final Logger LOG = LoggerFactory.getLogger(
CapacitySchedulerConfigValidator.class);
+ private interface QueueChangeValidator {
+ void validate(CSQueue oldQueue, CSQueue newQueue) throws IOException;
+ }
+
+ private static final List<QueueChangeValidator> QUEUE_CHANGE_VALIDATORS =
Arrays.asList(
Review comment:
This is a good idea, I like it, however I think it would be a better to
define the validators as static methods instead, because you are basically
using them as functions anyway.
--
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]
