You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "jpcorreia99 (via GitHub)" <gi...@apache.org> on 2024/02/28 10:39:26 UTC
Re: [PR] [SPARK-47208] Allow overriding base overhead memory [spark]
jpcorreia99 commented on code in PR #45240:
URL: https://github.com/apache/spark/pull/45240#discussion_r1505736480
##########
core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala:
##########
@@ -489,10 +489,13 @@ object ResourceProfile extends Logging {
private[spark] def calculateOverHeadMemory(
overHeadMemFromConf: Option[Long],
+ minimumOverHeadMemoryFromConf: Option[Long],
executorMemoryMiB: Long,
overheadFactor: Double): Long = {
+ val minMemoryOverhead =
+ minimumOverHeadMemoryFromConf.getOrElse(ResourceProfile.MEMORY_OVERHEAD_MIN_MIB);
overHeadMemFromConf.getOrElse(math.max((overheadFactor * executorMemoryMiB).toInt,
- ResourceProfile.MEMORY_OVERHEAD_MIN_MIB))
+ minMemoryOverhead))
Review Comment:
I order to minimize the changes to existing logic, I chose to make this flag not have a default and instead write the logic to check if it is not set and use the existing constant.
However, from a code cleaniness and maintenance standpoint, I think the constant can be deleted and the flag value defaulted to 384Mib.
@the reviewer, what's your view here?
##########
core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala:
##########
@@ -489,10 +489,13 @@ object ResourceProfile extends Logging {
private[spark] def calculateOverHeadMemory(
overHeadMemFromConf: Option[Long],
+ minimumOverHeadMemoryFromConf: Option[Long],
executorMemoryMiB: Long,
overheadFactor: Double): Long = {
+ val minMemoryOverhead =
+ minimumOverHeadMemoryFromConf.getOrElse(ResourceProfile.MEMORY_OVERHEAD_MIN_MIB);
overHeadMemFromConf.getOrElse(math.max((overheadFactor * executorMemoryMiB).toInt,
- ResourceProfile.MEMORY_OVERHEAD_MIN_MIB))
+ minMemoryOverhead))
Review Comment:
I order to minimize the changes to existing logic, I chose to make this flag not have a default and instead write the logic to check if it is not set and use the existing constant.
However, from a code cleaniness and maintenance standpoint, I think the constant can be deleted and the flag value defaulted to 384Mib.
@reviewer, what's your view here?
--
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