9uapaw commented on a change in pull request #3475:
URL: https://github.com/apache/hadoop/pull/3475#discussion_r721071929
##########
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:
Also found that shouldAppFailFast is a strange method, that could be
either:
1. Defined only in the config class without a parameter
2. Defined as a static method
I have chosen the latter, as I am not sure about how each configuration is
used in CS (there are 3 of them) and I would refrain from altering it as of now
(it could lead to really subtle issues).
--
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]
