You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2021/10/01 03:02:20 UTC
[GitHub] [hadoop] shuzirra commented on a change in pull request #3475: YARN-10953. Make CapacityScheduler#getOrCreateQueueFromPlacementConte…
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org