Fix MB units conversion(spark->k8s)&Set the executor ENV_EXECUTOR_MEMORY to executorMemoryWithOverhead #471
Conversation
…MORY` environment variable to `executorMemoryWithOverhead` in accordance with driver. Signed-off-by: duyanghao <[email protected]>
duyanghao
changed the title
ENV_EXECUTOR_MEMORY to executorMemoryWithOverhead in accordance with driver.
|
Dupe of #470 |
| // Executor backend expects integral value for executor cores, so round it up to an int. | ||
| (ENV_EXECUTOR_CORES, math.ceil(executorCores).toInt.toString), | ||
| (ENV_EXECUTOR_MEMORY, executorMemoryString), | ||
| (ENV_EXECUTOR_MEMORY, executorMemoryWithOverhead + "m"), |
There was a problem hiding this comment.
I don't think we want to do this. The point of the memory with overhead is to make it higher than what we give the JVM. The JVM should only have the exact amount of heap given by spark.executor.memory, but the overhead accounts for the fact that the JVM might also allocate memory off-heap, and in Python's case it also covers for the storage of Python's data.
Hence I think #470 does cover everything we need here.
There was a problem hiding this comment.
@ash211 @mccheah as i said in #467, Right now, the ENV_EXECUTOR_MEMORY is memory but ENV_DRIVER_MEMORY is memory+overhead
If the ENV_EXECUTOR_MEMORY should only be memory, as you said above, the ENV_DRIVER_MEMORY should also only be memory rather than memory+overhead,is that right?
So i suggest that ENV_DRIVER_MEMORY should also be just memory instead of memory+overheads.
I have created a PR for to correct this problem.
### Upstream SPARK-XXXXX ticket and PR link (if not applicable, explain) No upstream PR - SafeLogging is palantir specific. ### What changes were proposed in this pull request? If the log in SafeLogging is not marked as transient, it is included in the serialized version of TorrentBroadcast, which causes issues. This excludes the log from being included in the TorrentBroadcast object, by making the log transient. ### How was this patch tested? No testing for this - the logic is taken from upstream
3 participants
What changes were proposed in this pull request?
ENV_EXECUTOR_MEMORYenvironment variable toexecutorMemoryWithOverheadin accordance with driver.How was this patch tested?
Manual tests show that Memory units conversion is correct and
ENV_EXECUTOR_MEMORYenvironment variable is same withexecutorMemoryWithOverhead.