You are viewing a plain text version of this content. The canonical link for it is here.
Posted to yarn-issues@hadoop.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2023/02/01 16:48:00 UTC
[jira] [Commented] (YARN-11416) FS2CS should use CapacitySchedulerConfiguration in FSQueueConverterBuilder
[ https://issues.apache.org/jira/browse/YARN-11416?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17683092#comment-17683092 ]
ASF GitHub Bot commented on YARN-11416:
---------------------------------------
brumi1024 commented on code in PR #5320:
URL: https://github.com/apache/hadoop/pull/5320#discussion_r1093432627
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -723,6 +727,17 @@ public <S extends SchedulableEntity> OrderingPolicy<S> getAppOrderingPolicy(
return orderingPolicy;
}
+ public void setOrderingPolicy(String queue,
+ String appOrderingPolicy, String postfix, boolean value) {
+ setBoolean(getQueuePrefix(queue) +
+ "ordering-policy" + DOT + appOrderingPolicy + DOT + postfix, value);
+ }
+
+ public boolean getOrderingPolicy(String queue, String appOrderingPolicy, String postfix) {
+ return getBoolean(getQueuePrefix(queue) +
+ "ordering-policy" + DOT + appOrderingPolicy + DOT + postfix, false);
+ }
Review Comment:
This naming is really misleading, as it doesn't get the ordering policy, but it gets the properties of that policy. Since the ordering policy is pluggable, anyone can write one I think we shouldn't introduce getters for it, especially not fixed return type ones.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -536,6 +536,10 @@ public void setMaximumApplicationMasterResourcePerQueuePercent(String queue,
setFloat(getQueuePrefix(queue) + MAXIMUM_AM_RESOURCE_SUFFIX, percent);
}
+ public void setMaximumApplicationMasterResourcePercent(float percent) {
Review Comment:
Nit: a getter exists for this property, could you please move and rename this setter to be more in line of that method? It's called: getMaximumApplicationMasterResourcePercent
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1203,6 +1218,16 @@ public void reinitializeConfigurationProperties() {
configurationProperties = new ConfigurationProperties(props);
}
+ public void setQueueMaximumAllocationMb(String queue, String value) {
+ String queuePrefix = getQueuePrefix(queue);
+ set(queuePrefix + "maximum-allocation-mb", value);
Review Comment:
Please use the static constants instead of literals.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1693,6 +1726,14 @@ public boolean getIntraQueuePreemptionDisabled(String queue,
+ QUEUE_PREEMPTION_DISABLED, defaultVal);
}
+ public void setDisablePreemptionForPreemtionModes(String preemptionType, boolean value) {
+ setBoolean(PREEMPTION_CONFIG_PREFIX + preemptionType, value);
+ }
+
+ public boolean getDisablePreemptionForObserveOnly() {
Review Comment:
getDisablePreemptionForObserveOnly -> getPreemptionObserveOnly
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1693,6 +1726,14 @@ public boolean getIntraQueuePreemptionDisabled(String queue,
+ QUEUE_PREEMPTION_DISABLED, defaultVal);
}
+ public void setDisablePreemptionForPreemtionModes(String preemptionType, boolean value) {
+ setBoolean(PREEMPTION_CONFIG_PREFIX + preemptionType, value);
+ }
Review Comment:
The property observe only is a valid one, however it's not connected to disabling preemption itself, so the naming is misleading. And observe_only is a property of preemption, I wouldn't call it a type. Also please don't expect the property name as a parameter, because it destroys the purpose of the setter.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1794,29 +1835,48 @@ public static boolean shouldAppFailFast(Configuration conf) {
return conf.getBoolean(APP_FAIL_FAST, DEFAULT_APP_FAIL_FAST);
}
- public Integer getMaxParallelAppsForQueue(String queue) {
- int defaultMaxParallelAppsForQueue =
- getInt(PREFIX + MAX_PARALLEL_APPLICATIONS,
- DEFAULT_MAX_PARALLEL_APPLICATIONS);
+ public void setQueueMaxParallelApplications(String value) {
+ set(PREFIX + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setQueueMaxParallelApplications(String queue, String value) {
Review Comment:
It's generally a good idea to name the getters and setters exactly the same apart from the prefix: getMaxParallelAppsForQueue -> setMaxParallelAppsForQueue
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1794,29 +1835,48 @@ public static boolean shouldAppFailFast(Configuration conf) {
return conf.getBoolean(APP_FAIL_FAST, DEFAULT_APP_FAIL_FAST);
}
- public Integer getMaxParallelAppsForQueue(String queue) {
- int defaultMaxParallelAppsForQueue =
- getInt(PREFIX + MAX_PARALLEL_APPLICATIONS,
- DEFAULT_MAX_PARALLEL_APPLICATIONS);
+ public void setQueueMaxParallelApplications(String value) {
+ set(PREFIX + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setQueueMaxParallelApplications(String queue, String value) {
+ set(getQueuePrefix(queue) + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setUserMaxParallelApplications(String value) {
+ set(PREFIX + "user." + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setUserMaxParallelApplications(String user, int value) {
Review Comment:
setUserMaxParallelApplications -> setMaxParallelAppsForUser
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1794,29 +1835,48 @@ public static boolean shouldAppFailFast(Configuration conf) {
return conf.getBoolean(APP_FAIL_FAST, DEFAULT_APP_FAIL_FAST);
}
- public Integer getMaxParallelAppsForQueue(String queue) {
- int defaultMaxParallelAppsForQueue =
- getInt(PREFIX + MAX_PARALLEL_APPLICATIONS,
- DEFAULT_MAX_PARALLEL_APPLICATIONS);
+ public void setQueueMaxParallelApplications(String value) {
+ set(PREFIX + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setQueueMaxParallelApplications(String queue, String value) {
+ set(getQueuePrefix(queue) + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setUserMaxParallelApplications(String value) {
Review Comment:
Same as my comment at setQueueMaxParallelApplications: this could be renamed to setDefaultMaxParallelAppsPerUser
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1203,6 +1218,16 @@ public void reinitializeConfigurationProperties() {
configurationProperties = new ConfigurationProperties(props);
}
+ public void setQueueMaximumAllocationMb(String queue, String value) {
+ String queuePrefix = getQueuePrefix(queue);
+ set(queuePrefix + "maximum-allocation-mb", value);
+ }
+
+ public void setQueueMaximumAllocationVcores(String queue, String value) {
+ String queuePrefix = getQueuePrefix(queue);
+ set(queuePrefix + "maximum-allocation-vcores", value);
Review Comment:
Same as above.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1794,29 +1835,48 @@ public static boolean shouldAppFailFast(Configuration conf) {
return conf.getBoolean(APP_FAIL_FAST, DEFAULT_APP_FAIL_FAST);
}
- public Integer getMaxParallelAppsForQueue(String queue) {
- int defaultMaxParallelAppsForQueue =
- getInt(PREFIX + MAX_PARALLEL_APPLICATIONS,
- DEFAULT_MAX_PARALLEL_APPLICATIONS);
+ public void setQueueMaxParallelApplications(String value) {
+ set(PREFIX + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setQueueMaxParallelApplications(String queue, String value) {
+ set(getQueuePrefix(queue) + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setUserMaxParallelApplications(String value) {
+ set(PREFIX + "user." + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setUserMaxParallelApplications(String user, int value) {
+ setInt(getUserPrefix(user) + MAX_PARALLEL_APPLICATIONS, value);
+ }
+ public Integer getMaxParallelAppsForQueue(String queue) {
String maxParallelAppsForQueue = get(getQueuePrefix(queue)
+ MAX_PARALLEL_APPLICATIONS);
return (maxParallelAppsForQueue != null) ?
Integer.parseInt(maxParallelAppsForQueue)
- : defaultMaxParallelAppsForQueue;
+ : getMaxParallelAppsForQueue();
}
- public Integer getMaxParallelAppsForUser(String user) {
- int defaultMaxParallelAppsForUser =
- getInt(PREFIX + "user." + MAX_PARALLEL_APPLICATIONS,
+ public Integer getMaxParallelAppsForQueue() {
+ return getInt(PREFIX + MAX_PARALLEL_APPLICATIONS,
DEFAULT_MAX_PARALLEL_APPLICATIONS);
+ }
Review Comment:
Just like the setter, the getters could be renamed accordingly.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1794,29 +1835,48 @@ public static boolean shouldAppFailFast(Configuration conf) {
return conf.getBoolean(APP_FAIL_FAST, DEFAULT_APP_FAIL_FAST);
}
- public Integer getMaxParallelAppsForQueue(String queue) {
- int defaultMaxParallelAppsForQueue =
- getInt(PREFIX + MAX_PARALLEL_APPLICATIONS,
- DEFAULT_MAX_PARALLEL_APPLICATIONS);
+ public void setQueueMaxParallelApplications(String value) {
Review Comment:
Since this property is essentially the fallback default value of the per queue max parallel applications I suggest renaming it accordingly. I.e: setDefaultMaxParallelAppsPerQueue
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -1794,29 +1835,48 @@ public static boolean shouldAppFailFast(Configuration conf) {
return conf.getBoolean(APP_FAIL_FAST, DEFAULT_APP_FAIL_FAST);
}
- public Integer getMaxParallelAppsForQueue(String queue) {
- int defaultMaxParallelAppsForQueue =
- getInt(PREFIX + MAX_PARALLEL_APPLICATIONS,
- DEFAULT_MAX_PARALLEL_APPLICATIONS);
+ public void setQueueMaxParallelApplications(String value) {
+ set(PREFIX + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setQueueMaxParallelApplications(String queue, String value) {
+ set(getQueuePrefix(queue) + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setUserMaxParallelApplications(String value) {
+ set(PREFIX + "user." + MAX_PARALLEL_APPLICATIONS, value);
+ }
+
+ public void setUserMaxParallelApplications(String user, int value) {
+ setInt(getUserPrefix(user) + MAX_PARALLEL_APPLICATIONS, value);
+ }
+ public Integer getMaxParallelAppsForQueue(String queue) {
String maxParallelAppsForQueue = get(getQueuePrefix(queue)
+ MAX_PARALLEL_APPLICATIONS);
return (maxParallelAppsForQueue != null) ?
Integer.parseInt(maxParallelAppsForQueue)
- : defaultMaxParallelAppsForQueue;
+ : getMaxParallelAppsForQueue();
}
- public Integer getMaxParallelAppsForUser(String user) {
- int defaultMaxParallelAppsForUser =
- getInt(PREFIX + "user." + MAX_PARALLEL_APPLICATIONS,
+ public Integer getMaxParallelAppsForQueue() {
+ return getInt(PREFIX + MAX_PARALLEL_APPLICATIONS,
DEFAULT_MAX_PARALLEL_APPLICATIONS);
+ }
+
+ public Integer getMaxParallelAppsForUser(String user) {
String maxParallelAppsForUser = get(getUserPrefix(user)
+ MAX_PARALLEL_APPLICATIONS);
return (maxParallelAppsForUser != null) ?
Integer.parseInt(maxParallelAppsForUser)
- : defaultMaxParallelAppsForUser;
+ : getMaxParallelAppsForUser();
+ }
+
+ public Integer getMaxParallelAppsForUser() {
+ return getInt(PREFIX + "user." + MAX_PARALLEL_APPLICATIONS,
+ DEFAULT_MAX_PARALLEL_APPLICATIONS);
}
Review Comment:
Same as above.
> FS2CS should use CapacitySchedulerConfiguration in FSQueueConverterBuilder
> ---------------------------------------------------------------------------
>
> Key: YARN-11416
> URL: https://issues.apache.org/jira/browse/YARN-11416
> Project: Hadoop YARN
> Issue Type: Improvement
> Reporter: Benjamin Teke
> Assignee: Susheel Gupta
> Priority: Major
> Labels: pull-request-available
>
> org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.converter.FSQueueConverter and its builder stores the variable capacitySchedulerConfig as a simple Configuration object instead of CapacitySchedulerConfiguration. This is misleading, as capacitySchedulerConfig suggests that it is indeed a CapacitySchedulerConfiguration and it loses access to the convenience methods to check for various properties. Because of this every time a property getter is changed FS2CS should be checked if it reimplemented the same, otherwise there might be behaviour differences or even bugs.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org