shuzirra commented on a change in pull request #3475:
URL: https://github.com/apache/hadoop/pull/3475#discussion_r719899219
##########
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/CapacityScheduler.java
##########
@@ -951,73 +951,79 @@ private CSQueue
getOrCreateQueueFromPlacementContext(ApplicationId
applicationId, String user, String queueName,
ApplicationPlacementContext placementContext,
boolean isRecovery) {
-
CSQueue queue = getQueue(queueName);
ApplicationPlacementContext fallbackContext = placementContext;
Review comment:
We don't need fallbackContext, we need a fallback QueuePath, we don't
actually use the placement context. See other comments why.
##########
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/CapacityScheduler.java
##########
@@ -951,73 +951,79 @@ private CSQueue
getOrCreateQueueFromPlacementContext(ApplicationId
applicationId, String user, String queueName,
ApplicationPlacementContext placementContext,
boolean isRecovery) {
-
CSQueue queue = getQueue(queueName);
ApplicationPlacementContext fallbackContext = placementContext;
- if (queue == null) {
- // Even if placement rules are turned off, we still have the opportunity
- // to auto create a queue.
- if (placementContext == null) {
- fallbackContext = CSQueueUtils.extractQueuePath(queueName);
- }
+ if (queue != null) {
+ return queue;
+ }
- //we need to make sure there is no empty path parts present
- String path = fallbackContext.getFullQueuePath();
- String[] pathParts = path.split("\\.");
- for (int i = 0; i < pathParts.length; i++) {
- if ("".equals(pathParts[i])) {
- LOG.error("Application submitted to invalid path: '{}'", path);
- return null;
- }
+ // Even if placement rules are turned off, we still have the opportunity
+ // to auto create a queue.
+ if (placementContext == null) {
+ fallbackContext = CSQueueUtils.extractQueuePath(queueName);
+ }
+
+ //we need to make sure there are no empty path parts present
+ QueuePath path = new QueuePath(fallbackContext.getFullQueuePath());
+ if (path.hasEmptyPart()) {
+ LOG.error("Application submitted to invalid path: '{}'", path);
+ return null;
+ }
+
+ if (!fallbackContext.hasParentQueue()) {
+ return null;
+ }
+
+ try {
+ writeLock.lock();
+ return queueManager.createQueue(fallbackContext);
Review comment:
We need to make a polymorphic version of this method (actually two, one
with the common parts, which accepts two strings, and one which accepts the
QueuePath object) which accepts the QueuePath object, internally this method
only uses parent / leaf names, and since we want to use QueuePath object more
often, it is a good opportunity, to start adding support for it.
##########
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/CapacityScheduler.java
##########
@@ -951,73 +951,79 @@ private CSQueue
getOrCreateQueueFromPlacementContext(ApplicationId
applicationId, String user, String queueName,
ApplicationPlacementContext placementContext,
boolean isRecovery) {
-
CSQueue queue = getQueue(queueName);
ApplicationPlacementContext fallbackContext = placementContext;
- if (queue == null) {
- // Even if placement rules are turned off, we still have the opportunity
- // to auto create a queue.
- if (placementContext == null) {
- fallbackContext = CSQueueUtils.extractQueuePath(queueName);
- }
+ if (queue != null) {
+ return queue;
+ }
- //we need to make sure there is no empty path parts present
- String path = fallbackContext.getFullQueuePath();
- String[] pathParts = path.split("\\.");
- for (int i = 0; i < pathParts.length; i++) {
- if ("".equals(pathParts[i])) {
- LOG.error("Application submitted to invalid path: '{}'", path);
- return null;
- }
+ // Even if placement rules are turned off, we still have the opportunity
+ // to auto create a queue.
+ if (placementContext == null) {
+ fallbackContext = CSQueueUtils.extractQueuePath(queueName);
+ }
+
+ //we need to make sure there are no empty path parts present
+ QueuePath path = new QueuePath(fallbackContext.getFullQueuePath());
+ if (path.hasEmptyPart()) {
+ LOG.error("Application submitted to invalid path: '{}'", path);
+ return null;
+ }
+
+ if (!fallbackContext.hasParentQueue()) {
+ return null;
+ }
+
Review comment:
Parent queue might be ambiguous as well, we either should check here or
in the createQueue method, to give more detailed report about the problem.
##########
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/CapacityScheduler.java
##########
@@ -951,73 +951,79 @@ private CSQueue
getOrCreateQueueFromPlacementContext(ApplicationId
applicationId, String user, String queueName,
ApplicationPlacementContext placementContext,
boolean isRecovery) {
-
CSQueue queue = getQueue(queueName);
ApplicationPlacementContext fallbackContext = placementContext;
- if (queue == null) {
- // Even if placement rules are turned off, we still have the opportunity
- // to auto create a queue.
- if (placementContext == null) {
- fallbackContext = CSQueueUtils.extractQueuePath(queueName);
- }
+ if (queue != null) {
+ return queue;
+ }
Review comment:
You might want to add an ambiguity check here
(queueManager.isAmbiguous), due to legacy reasons we still need to expect
queues referenced with non-full path, and if the short name of the queue is
ambiguous, getQueue will return null. And perhaps we can give the user a more
detailed reason if this is the case.
##########
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/CapacityScheduler.java
##########
@@ -951,73 +951,79 @@ private CSQueue
getOrCreateQueueFromPlacementContext(ApplicationId
applicationId, String user, String queueName,
ApplicationPlacementContext placementContext,
boolean isRecovery) {
-
CSQueue queue = getQueue(queueName);
ApplicationPlacementContext fallbackContext = placementContext;
- if (queue == null) {
- // Even if placement rules are turned off, we still have the opportunity
- // to auto create a queue.
- if (placementContext == null) {
- fallbackContext = CSQueueUtils.extractQueuePath(queueName);
- }
+ if (queue != null) {
+ return queue;
+ }
- //we need to make sure there is no empty path parts present
- String path = fallbackContext.getFullQueuePath();
- String[] pathParts = path.split("\\.");
- for (int i = 0; i < pathParts.length; i++) {
- if ("".equals(pathParts[i])) {
- LOG.error("Application submitted to invalid path: '{}'", path);
- return null;
- }
+ // Even if placement rules are turned off, we still have the opportunity
+ // to auto create a queue.
+ if (placementContext == null) {
+ fallbackContext = CSQueueUtils.extractQueuePath(queueName);
+ }
+
+ //we need to make sure there are no empty path parts present
+ QueuePath path = new QueuePath(fallbackContext.getFullQueuePath());
+ if (path.hasEmptyPart()) {
+ LOG.error("Application submitted to invalid path: '{}'", path);
+ return null;
+ }
+
+ if (!fallbackContext.hasParentQueue()) {
+ return null;
+ }
+
+ try {
+ writeLock.lock();
+ return queueManager.createQueue(fallbackContext);
+ } catch (YarnException | IOException e) {
+ // A null queue is expected if the placementContext is null. In order
+ // not to disrupt the control flow, if we fail to auto create a queue,
+ // we fall back to the original logic.
+ if (placementContext == null) {
+ LOG.error("Could not auto-create leaf queue " + queueName +
+ " due to : ", e);
+ return null;
}
+ handleQueueCreationError(applicationId, user, queueName, isRecovery, e);
+ } finally {
+ writeLock.unlock();
+ }
+ return null;
+ }
- if (fallbackContext.hasParentQueue()) {
- try {
- writeLock.lock();
- return queueManager.createQueue(fallbackContext);
- } catch (YarnException | IOException e) {
- // A null queue is expected if the placementContext is null. In order
- // not to disrupt the control flow, if we fail to auto create a
queue,
- // we fall back to the original logic.
- if (placementContext == null) {
- LOG.error("Could not auto-create leaf queue " + queueName +
- " due to : ", e);
- return null;
- }
- if (isRecovery) {
- if (!getConfiguration().shouldAppFailFast(getConfig())) {
- LOG.error("Could not auto-create leaf queue " + queueName +
- " due to : ", e);
- this.rmContext.getDispatcher().getEventHandler().handle(
- new RMAppEvent(applicationId, RMAppEventType.KILL,
- "Application killed on recovery"
- + " as it was submitted to queue " + queueName
- + " which could not be auto-created"));
- } else{
- String queueErrorMsg =
- "Queue named " + queueName + " could not be "
- + "auto-created during application recovery.";
- LOG.error(FATAL, queueErrorMsg, e);
- throw new QueueInvalidException(queueErrorMsg);
- }
- } else{
- LOG.error("Could not auto-create leaf queue due to : ", e);
- final String message =
- "Application " + applicationId + " submission by user : "
- + user
- + " to queue : " + queueName + " failed : " + e
- .getMessage();
- this.rmContext.getDispatcher().getEventHandler().handle(
- new RMAppEvent(applicationId, RMAppEventType.APP_REJECTED,
- message));
- }
- } finally {
- writeLock.unlock();
- }
+ private void handleQueueCreationError(
+ ApplicationId applicationId, String user, String queueName,
+ boolean isRecovery, Exception e) {
+ if (isRecovery) {
+ if (!getConfiguration().shouldAppFailFast(getConfig())) {
+ LOG.error("Could not auto-create leaf queue " + queueName +
+ " due to : ", e);
+ this.rmContext.getDispatcher().getEventHandler().handle(
+ new RMAppEvent(applicationId, RMAppEventType.KILL,
+ "Application killed on recovery"
+ + " as it was submitted to queue " + queueName
+ + " which could not be auto-created"));
Review comment:
which did not exist and could not be auto created.
##########
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/CapacityScheduler.java
##########
@@ -951,73 +951,79 @@ private CSQueue
getOrCreateQueueFromPlacementContext(ApplicationId
applicationId, String user, String queueName,
ApplicationPlacementContext placementContext,
boolean isRecovery) {
-
CSQueue queue = getQueue(queueName);
ApplicationPlacementContext fallbackContext = placementContext;
- if (queue == null) {
- // Even if placement rules are turned off, we still have the opportunity
- // to auto create a queue.
- if (placementContext == null) {
- fallbackContext = CSQueueUtils.extractQueuePath(queueName);
- }
+ if (queue != null) {
+ return queue;
+ }
- //we need to make sure there is no empty path parts present
- String path = fallbackContext.getFullQueuePath();
- String[] pathParts = path.split("\\.");
- for (int i = 0; i < pathParts.length; i++) {
- if ("".equals(pathParts[i])) {
- LOG.error("Application submitted to invalid path: '{}'", path);
- return null;
- }
+ // Even if placement rules are turned off, we still have the opportunity
+ // to auto create a queue.
+ if (placementContext == null) {
+ fallbackContext = CSQueueUtils.extractQueuePath(queueName);
Review comment:
We should simply create the fallbackQueuePath here, we only use the
fallbackContext, to create an instance of the QueuePath object, which has the
ability to be created from a full path, so instead of
CSQueueUtils.extractQueuePath(queueName); we can simply create the new
QueuePath here, which saves us the creation of an unused
ApplicationPlacementContext.
##########
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/CapacityScheduler.java
##########
@@ -951,73 +951,79 @@ private CSQueue
getOrCreateQueueFromPlacementContext(ApplicationId
applicationId, String user, String queueName,
ApplicationPlacementContext placementContext,
boolean isRecovery) {
-
CSQueue queue = getQueue(queueName);
ApplicationPlacementContext fallbackContext = placementContext;
- if (queue == null) {
- // Even if placement rules are turned off, we still have the opportunity
- // to auto create a queue.
- if (placementContext == null) {
- fallbackContext = CSQueueUtils.extractQueuePath(queueName);
- }
+ if (queue != null) {
+ return queue;
+ }
- //we need to make sure there is no empty path parts present
- String path = fallbackContext.getFullQueuePath();
- String[] pathParts = path.split("\\.");
- for (int i = 0; i < pathParts.length; i++) {
- if ("".equals(pathParts[i])) {
- LOG.error("Application submitted to invalid path: '{}'", path);
- return null;
- }
+ // Even if placement rules are turned off, we still have the opportunity
+ // to auto create a queue.
+ if (placementContext == null) {
+ fallbackContext = CSQueueUtils.extractQueuePath(queueName);
+ }
+
+ //we need to make sure there are no empty path parts present
+ QueuePath path = new QueuePath(fallbackContext.getFullQueuePath());
+ if (path.hasEmptyPart()) {
+ LOG.error("Application submitted to invalid path: '{}'", path);
+ return null;
+ }
+
+ if (!fallbackContext.hasParentQueue()) {
+ return null;
+ }
+
+ try {
+ writeLock.lock();
+ return queueManager.createQueue(fallbackContext);
+ } catch (YarnException | IOException e) {
+ // A null queue is expected if the placementContext is null. In order
+ // not to disrupt the control flow, if we fail to auto create a queue,
+ // we fall back to the original logic.
+ if (placementContext == null) {
+ LOG.error("Could not auto-create leaf queue " + queueName +
+ " due to : ", e);
+ return null;
}
+ handleQueueCreationError(applicationId, user, queueName, isRecovery, e);
+ } finally {
+ writeLock.unlock();
+ }
+ return null;
+ }
- if (fallbackContext.hasParentQueue()) {
- try {
- writeLock.lock();
- return queueManager.createQueue(fallbackContext);
- } catch (YarnException | IOException e) {
- // A null queue is expected if the placementContext is null. In order
- // not to disrupt the control flow, if we fail to auto create a
queue,
- // we fall back to the original logic.
- if (placementContext == null) {
- LOG.error("Could not auto-create leaf queue " + queueName +
- " due to : ", e);
- return null;
- }
- if (isRecovery) {
- if (!getConfiguration().shouldAppFailFast(getConfig())) {
- LOG.error("Could not auto-create leaf queue " + queueName +
- " due to : ", e);
- this.rmContext.getDispatcher().getEventHandler().handle(
- new RMAppEvent(applicationId, RMAppEventType.KILL,
- "Application killed on recovery"
- + " as it was submitted to queue " + queueName
- + " which could not be auto-created"));
- } else{
- String queueErrorMsg =
- "Queue named " + queueName + " could not be "
- + "auto-created during application recovery.";
- LOG.error(FATAL, queueErrorMsg, e);
- throw new QueueInvalidException(queueErrorMsg);
- }
- } else{
- LOG.error("Could not auto-create leaf queue due to : ", e);
- final String message =
- "Application " + applicationId + " submission by user : "
- + user
- + " to queue : " + queueName + " failed : " + e
- .getMessage();
- this.rmContext.getDispatcher().getEventHandler().handle(
- new RMAppEvent(applicationId, RMAppEventType.APP_REJECTED,
- message));
- }
- } finally {
- writeLock.unlock();
- }
+ private void handleQueueCreationError(
+ ApplicationId applicationId, String user, String queueName,
+ boolean isRecovery, Exception e) {
+ if (isRecovery) {
+ if (!getConfiguration().shouldAppFailFast(getConfig())) {
Review comment:
Couldn't we save it as a member variable, it seems a bit unnecessary to
have 3 method calls, and a map lookup, just for getting a simple flag. We might
need to make sure that the flag is recalculated on config reload tho.
##########
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/CapacityScheduler.java
##########
@@ -951,73 +951,79 @@ private CSQueue
getOrCreateQueueFromPlacementContext(ApplicationId
applicationId, String user, String queueName,
ApplicationPlacementContext placementContext,
boolean isRecovery) {
-
CSQueue queue = getQueue(queueName);
ApplicationPlacementContext fallbackContext = placementContext;
- if (queue == null) {
- // Even if placement rules are turned off, we still have the opportunity
- // to auto create a queue.
- if (placementContext == null) {
- fallbackContext = CSQueueUtils.extractQueuePath(queueName);
- }
+ if (queue != null) {
+ return queue;
+ }
- //we need to make sure there is no empty path parts present
- String path = fallbackContext.getFullQueuePath();
- String[] pathParts = path.split("\\.");
- for (int i = 0; i < pathParts.length; i++) {
- if ("".equals(pathParts[i])) {
- LOG.error("Application submitted to invalid path: '{}'", path);
- return null;
- }
+ // Even if placement rules are turned off, we still have the opportunity
+ // to auto create a queue.
+ if (placementContext == null) {
+ fallbackContext = CSQueueUtils.extractQueuePath(queueName);
+ }
+
+ //we need to make sure there are no empty path parts present
+ QueuePath path = new QueuePath(fallbackContext.getFullQueuePath());
+ if (path.hasEmptyPart()) {
+ LOG.error("Application submitted to invalid path: '{}'", path);
Review comment:
Please specify why is it an invalid path (it has empty parts)
##########
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/CapacityScheduler.java
##########
@@ -951,73 +951,79 @@ private CSQueue
getOrCreateQueueFromPlacementContext(ApplicationId
applicationId, String user, String queueName,
ApplicationPlacementContext placementContext,
boolean isRecovery) {
-
CSQueue queue = getQueue(queueName);
ApplicationPlacementContext fallbackContext = placementContext;
- if (queue == null) {
- // Even if placement rules are turned off, we still have the opportunity
- // to auto create a queue.
- if (placementContext == null) {
- fallbackContext = CSQueueUtils.extractQueuePath(queueName);
- }
+ if (queue != null) {
+ return queue;
+ }
- //we need to make sure there is no empty path parts present
- String path = fallbackContext.getFullQueuePath();
- String[] pathParts = path.split("\\.");
- for (int i = 0; i < pathParts.length; i++) {
- if ("".equals(pathParts[i])) {
- LOG.error("Application submitted to invalid path: '{}'", path);
- return null;
- }
+ // Even if placement rules are turned off, we still have the opportunity
+ // to auto create a queue.
+ if (placementContext == null) {
+ fallbackContext = CSQueueUtils.extractQueuePath(queueName);
+ }
+
+ //we need to make sure there are no empty path parts present
+ QueuePath path = new QueuePath(fallbackContext.getFullQueuePath());
+ if (path.hasEmptyPart()) {
+ LOG.error("Application submitted to invalid path: '{}'", path);
+ return null;
+ }
+
+ if (!fallbackContext.hasParentQueue()) {
Review comment:
QueuePath has this feature as well.
--
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]
