LuciferYang commented on code in PR #12617:
URL: https://github.com/apache/gluten/pull/12617#discussion_r3656439264
##########
gluten-core/src/main/scala/org/apache/spark/util/SparkResourceUtil.scala:
##########
@@ -78,7 +78,17 @@ object SparkResourceUtil extends Logging {
def getTaskSlots(conf: SparkConf): Int = {
val executorCores = SparkResourceUtil.getExecutorCores(conf)
val taskCores = conf.getInt("spark.task.cpus", 1)
- executorCores / taskCores
+ if (taskCores <= 0) {
Review Comment:
The check isn't redundant, for two reasons.
First, on the Spark versions Gluten targets, `spark.task.cpus` isn't
validated at set time. The `checkValue(_ > 0)` on `CPUS_PER_TASK` was only
added in SPARK-55757, which ships in Spark 4.2. I decompiled spark-core 3.5.5
to confirm: its `CPUS_PER_TASK` is
`ConfigBuilder("spark.task.cpus").version("0.5.0").intConf.createWithDefault(1)`,
with no `checkValue`. So on Spark 3.3 through 4.1, nothing rejects a
non-positive value before we read it.
Second, even on 4.2+ where the check exists, it only fires on a typed
`conf.get(CPUS_PER_TASK)`. The first such read is in
`SparkContext.createTaskScheduler`, which runs after `PluginContainer` init.
`getTaskSlots` is reached through that plugin init (`GlutenDriverPlugin.init`
then `setPredefinedConfigs`), and it reads the value raw via
`conf.getInt("spark.task.cpus", 1)`, which skips the `ConfigEntry`. So we read
the raw value before Spark validates it, on every version.
I verified this on a real driver init (`new SparkContext` with
`spark.plugins=org.apache.gluten.GlutenPlugin`): `spark.task.cpus=0` throws
`ArithmeticException: / by zero` inside `setPredefinedConfigs`, and a negative
value silently produces negative task slots and negative per-task off-heap
budgets while the context still starts.
Based on your comment I switched the fix from coercing to 1 to failing fast
with `require(taskCores > 0, ...)`. A non-positive value is a real
misconfiguration that should surface rather than be silently rewritten, which
also matches the direction Spark took in 4.2. Just pushed the update, along
with a refreshed PR description and issue rationale.
--
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]
