[GitHub] [spark] tgravescs commented on a change in pull request #35901: [SPARK-38194] Followup: Fix k8s memory overhead passing to executor pods

[GitBox <gi...@apache.org>]

tgravescs commented on a change in pull request #35901:
URL: https://github.com/apache/spark/pull/35901#discussion_r829372761



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala
##########
@@ -53,28 +53,31 @@ private[spark] class BasicDriverFeatureStep(conf: KubernetesDriverConf)
 
   // Memory settings
   private val driverMemoryMiB = conf.get(DRIVER_MEMORY)
-  private val memoryOverheadFactor = if (conf.contains(DRIVER_MEMORY_OVERHEAD_FACTOR)) {
-    conf.get(DRIVER_MEMORY_OVERHEAD_FACTOR)
-  } else {
-    conf.get(MEMORY_OVERHEAD_FACTOR)
-  }
 
-  // The memory overhead factor to use. If the user has not set it, then use a different
-  // value for non-JVM apps. This value is propagated to executors.
-  private val overheadFactor =
+  // The default memory overhead factor to use. If the user has not set it, then use a different
+  // value for non-JVM apps. This value is propagated to executors and used if the executor
+  // overhead factor is not set explicitly.
+  private val defaultOverheadFactor =

Review comment:
       yes this is exactly what I was thinking originally, but then I realized that if the default values for driver vs executor were to ever change ion the new configs this wouldn't handle that.  So that is why it might actually be better to check in the BasicExecutorFeatureStep.   I'm ok with doing that as a followup though as this fixes the issue and both have the same default.
   
   one thing we may want to add comment above saying this is either the setting of old config or the default.




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org