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 "brumi1024 (via GitHub)" <gi...@apache.org> on 2023/02/01 16:47:11 UTC
[GitHub] [hadoop] brumi1024 commented on a diff in pull request #5320: YARN-11416. FS2CS should use CapacitySchedulerConfiguration in FSQueueConverterBuilder
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.
--
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