viirya commented on code in PR #966:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/966#discussion_r4267814793
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadFactory.java:
##########
@@ -301,17 +321,49 @@ static PodSet buildDriverPodSet(
}
static PodSet buildExecutorPodSet(
- final SparkApplication app, final Map<String, String> sparkConf) {
+ final SparkApplication app,
+ final Map<String, String> sparkConf,
+ final boolean dynamicAllocation) {
ApplicationSpec spec = app.getSpec();
return buildPodSet(
PODSET_EXECUTOR,
- parseInt(sparkConf.get("spark.executor.instances"),
DEFAULT_EXECUTOR_INSTANCES),
+ dynamicAllocation
+ ? getReservedMaxExecutors(app, sparkConf)
+ : parseInt(sparkConf.get("spark.executor.instances"),
DEFAULT_EXECUTOR_INSTANCES),
spec.getExecutorSpec(),
sparkConf,
Constants.EXECUTOR_SPARK_CONTAINER_PROP_KEY,
calculateExecutorMemoryMiB(sparkConf, isNonJvmApp(spec),
isPythonApp(spec)));
}
+ /**
+ * Returns `spark.dynamicAllocation.maxExecutors`, whose quota the Workload
of an application with
+ * dynamic allocation reserves if the application is annotated to, since the
quota of an admitted
+ * Workload cannot grow or shrink.
+ */
+ private static int getReservedMaxExecutors(
+ final SparkApplication app, final Map<String, String> sparkConf) {
+ Map<String, String> annotations = app.getMetadata().getAnnotations();
+ if (annotations == null
+ || !"true".equalsIgnoreCase(
+
annotations.get(Constants.ANNOTATION_KUEUE_RESERVE_MAX_EXECUTORS))) {
+ throw new UnsupportedOperationException(
+ "Kueue does not support SparkApplication with dynamic allocation "
+ + "(spark.dynamicAllocation.enabled=true) unless it is annotated
with "
+ + Constants.ANNOTATION_KUEUE_RESERVE_MAX_EXECUTORS
+ + "=true to reserve the quota of
spark.dynamicAllocation.maxExecutors executors.");
+ }
+ // Like Spark, the maximum is unlimited by default
+ int maxExecutors =
Review Comment:
Thanks for the correction, you're right —
`KubernetesClusterSchedulerBackend` calls
`SchedulerBackendUtils.getInitialTargetExecutorNumber`, which requires `initial
<= max` before the `ExecutorAllocationManager` is even created, so there's no
overrun. Rejecting it upfront is still a nice improvement.
--
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]
