[SPARK-28577][YARN]Resource capability requested for each executor add offHeapMemorySize #25309

LuciferYang · 2019-07-31T06:30:53Z

What changes were proposed in this pull request?

If MEMORY_OFFHEAP_ENABLED is true, add MEMORY_OFFHEAP_SIZE to resource requested for executor to ensure instance has enough memory to use.

In this pr add a helper method executorOffHeapMemorySizeAsMb in YarnSparkHadoopUtil.

How was this patch tested?

Add 3 new test suite to test YarnSparkHadoopUtil#executorOffHeapMemorySizeAsMb

…offHeapSize when offHeap is enable

LuciferYang · 2019-07-31T06:36:36Z

cc @xuanyuanking

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala

LuciferYang · 2019-07-31T10:53:21Z

@kiszk Thx for ur review ~
commit c44a33e fix this

xuanyuanking

+1 for choosing a safer config, just one nit for the log. cc @jerryshao.

xuanyuanking · 2019-08-02T11:13:19Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala

+        sparkConf.getSizeAsMb(MEMORY_OFFHEAP_SIZE.key, MEMORY_OFFHEAP_SIZE.defaultValueString)
+      require(size > 0,
+        s"${MEMORY_OFFHEAP_SIZE.key} must be > 0 when ${MEMORY_OFFHEAP_ENABLED.key} == true")
+      logInfo(s"${MEMORY_OFFHEAP_ENABLED.key} is true, ${MEMORY_OFFHEAP_SIZE.key} is $size, " +


How about only gives a warning log when we find the overhead is less than offHeap? We use the warning to notice user the changes of the config and explain why we change it, so there's no extra log for the same behavior as before.

@xuanyuanking Thx for ur review , in 9421fbe change to print a warn log when offHeapSize more than overhead

jerryshao · 2019-08-02T12:29:10Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala

+      }
+      size
+    } else 0
+    math.max(overhead, offHeap).toInt


I was wondering if it is better to change to overhead = overhead + offHeap if off-heap is enabled. Mainly because off heap memory is not only used for Spark itself related, but also for Netty and other native libraries. If we only guarantee overhead > offHeap, then it would somehow occupy the usage of Netty and others. Just my two cents :).

Got it ~, So should we add 2 field like isOffHeapEnabled and executorOffHeapMemory to YarnAllocator then use executorMemory + memoryOverhead + pysparkWorkerMemory + executorOffHeapMemory to request resource and no longer modify memoryOverhead?

Hmm, that's a bit complex as for now:

If we assume overhead memory includes all the off-heap memory Spark used (include everything). Then user should be aware of the different off-heap memory settings, and carefully set the overhead number to cover all the usages.

If we assume that overhead memory only related to some additional memory usage (not explicitly set by Spark, like off-heap memory). Then the overall executor memory should add all as mentioned above.

I think it would be better to involve other's opinion. CC @vanzin @tgravescs .

yeah I always thought this was a bit weird off heap was just included in the overhead, but never took the time to go back to see if it was discussed.

I think it's better to specifically add the off heap instead of include in the overhead. Just like we did for the pyspark memory. executorMemory + memoryOverhead + pysparkWorkerMemory + executorOffHeapMemory. I think that keeps things more consistent and obvious to the user.

@tgravescs Agree with you, overhead should be used to describe memory not use by Spark, like Netty used or JVM used as @jerryshao said, and we should clearly describe it in the configuration document.

So change to use executorMemory + memoryOverhead + pysparkWorkerMemory + executorOffHeapMemory to request resource?

@beliefer Now YarnAllocator line 150 use executorMemory + memoryOverhead + pysparkWorkerMemory to new Resource Instance, Is this wrong?

On the other hand, if the user configures offheapMemory and pysparkWorkerMemory,
He still needs to configure overhead Memroy and ensure that the configuration is reasonable(memoryOverhead > offheapMemory + pysparkWorkerMemory) in Yarn mode, so that users may need to care about more details.

@jerryshao Is the current approach feasible?

I have check the code and doc, there exists some inconsistent. According to the docs, memoryOverhead should comprise pysparkWorkerMemory. But the code have different behavior.
We need to fix the inconsistent. I think should reduce parameter to control memory, because more simple. @JoshRosen Could you take a look at this PR?

I agree with @tgravescs 's opinion.

yeah, I understand that, if we are going to change it, 3.0 is a good time to change that behavior. Like I said, I had found the off heap included in the overhead as confusing because you already had another separate config, why do I as a user have to add it into another config.

If overhead memory includes off-heap memory, pysparkWorkMemory and others, it makes user hard to set a proper overhead memory, users should know every other settings and figure out a proper number. As of time 3.0, I think we should give a good definition of overhead memory, it can be inconsistent with old version.

jerryshao · 2019-08-06T08:57:21Z

ok to test.

SparkQA · 2019-08-06T09:23:29Z

Test build #108704 has finished for PR 25309 at commit 4fb5362.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

beliefer · 2019-08-07T04:06:33Z

@jerryshao IMHO, Spark should reduce configuration parameters first. And then I think no matter what memory is, we use the unified parameter to control is better. Maybe separate parameter looks easy to understand.

jerryshao · 2019-08-08T09:41:40Z

@beliefer I'm a little confused, do you want a unified parameter, or separated parameters, could you explain more?

IMHO, Spark should reduce configuration parameters first. And then I think no matter what memory is, we use the unified parameter to control is better. Maybe separate parameter looks easy to understand.

jerryshao · 2019-08-08T09:49:05Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala

+      val sizeInMB =
+        sparkConf.getSizeAsMb(MEMORY_OFFHEAP_SIZE.key, MEMORY_OFFHEAP_SIZE.defaultValueString).toInt
+      require(sizeInMB > 0,
+        s"${MEMORY_OFFHEAP_SIZE.key} must be > 0 when ${MEMORY_OFFHEAP_ENABLED.key} == true")


Please check if MEMORY_OFFHEAP_SIZE could equal to 0. The definition of MEMORY_OFFHEAP_SIZE checks that it could be >= 0.

A little conflict with the MemoryManager as following:

if MEMORY_OFFHEAP_ENABLED is enable, MemoryManager.tungstenMemoryMode will enter OFF_HEAP branch and need MEMORY_OFFHEAP_SIZE > 0 and I think we should be consistent.

Then I think we should change the code here.

spark/core/src/main/scala/org/apache/spark/internal/config/package.scala

Line 234 in 1941d35

.checkValue(_ >= 0, "The off-heap memory size must not be negative")

0 is defaultValue, change to

.checkValue(_ > 0, "The off-heap memory size must be positive") .createWithDefault(1)

?
otherwise will throw IllegalArgumentException when offHeapEnabled is false and defaultValue is 0.

Maybe we should give a suitable defaultValue ,like 1073741824(1g)?

@jerryshao Do we need to change 0 to a suitable defaultValue?

I see, then I would suggest not to change it. Seems there's no good value which could cover most of the scenarios, so good to leave as it is.

its odd that check is >= 0 in the config, seems like we should change but can you file a separate jira for that?

OK~ I will add a new jira to discuss this issue.

jerryshao · 2019-08-08T09:53:04Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

      s"memory capability of the cluster ($maxMem MB per container)")
-    val executorMem = executorMemory + executorMemoryOverhead + pysparkWorkerMemory
+    val executorMem =
+      executorMemory + executorOffHeapMemory +executorMemoryOverhead + pysparkWorkerMemory


white space after +.

jerryshao · 2019-08-08T09:56:35Z

...rce-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtilSuite.scala

+    assert(executorOffHeapMemory == offHeapMemoryInMB)
+  }
+
+  test("executorMemoryOverhead when MEMORY_OFFHEAP_ENABLED is true, " +


Just wondering if we could add some yarn side UT to verify the container memory size, rather than verifying the correctness of off-heap configuration.

ok ~ I'll try to add it.

Add a new test suite SPARK-28577#YarnAllocator.resource.memory should include offHeapSize when offHeapEnabled is true. in YarnAllocatorSuite

SparkQA · 2019-08-08T12:46:57Z

Test build #108823 has finished for PR 25309 at commit c03db87.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

LuciferYang · 2019-08-22T07:12:12Z

@jerryshao Should we continue to complete this patch?

SparkQA · 2019-08-23T07:05:01Z

Test build #109620 has finished for PR 25309 at commit 40ad336.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2019-08-23T12:51:57Z

test this please

SparkQA · 2019-08-23T13:17:41Z

Test build #109642 has finished for PR 25309 at commit 40ad336.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2019-08-23T14:06:07Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala

+  def executorOffHeapMemorySizeAsMb(sparkConf: SparkConf): Int = {
+    if (sparkConf.get(MEMORY_OFFHEAP_ENABLED)) {
+      val sizeInMB =
+        Utils.byteStringAsMb(s"${sparkConf.get(MEMORY_OFFHEAP_SIZE)}B").toInt


you should be able to use memoryStringToMb instead.

// Convert to bytes, rather than directly to MiB, because when no units are specified the unit
// is assumed to be bytes

@tgravescs Thx for ur advice~

SparkQA · 2019-08-26T04:19:40Z

Test build #109713 has finished for PR 25309 at commit 0020a02.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jerryshao · 2019-08-26T06:34:23Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

      s"memory capability of the cluster ($maxMem MB per container)")
-    val executorMem = executorMemory + executorMemoryOverhead + pysparkWorkerMemory
+    val executorMem =
+      executorMemory + executorOffHeapMemory + executorMemoryOverhead + pysparkWorkerMemory


I think we should also update the doc to reflect the changes here.

ok, I'll try to update description about MemoryOverhead & OffHeapMemory in configuration.md, in this pr or new one ?

It would be better to change the doc in this PR.

b3b5f83 update the configuration.md.

There's some strange behavior about spark.executor.pyspark.memory:
if we config spark.executor.pyspark.memory , the pyspark executor memory is Independent , but if we not config spark.executor.pyspark.memory , the memoryOverhead include it.

jerryshao · 2019-08-26T06:36:54Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

+        s"offHeap memory ($executorOffHeapMemory) MB, overhead ($executorMemoryOverhead MB), " +
+        s"and PySpark memory ($pysparkWorkerMemory MB) is above the max threshold ($maxMem MB) " +
+        s"of this cluster! Please check the values of 'yarn.scheduler.maximum-allocation-mb' " +
+        s"and/or 'yarn.nodemanager.resource.memory-mb'.")


No need to add string interpolation for this line and above.

02c0f2a fix this

SparkQA · 2019-08-26T10:06:16Z

Test build #109729 has finished for PR 25309 at commit 02c0f2a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs

+1 looks good to me

SparkQA · 2019-08-27T07:05:01Z

Test build #109788 has finished for PR 25309 at commit b3b5f83.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2019-08-27T11:49:29Z

retest this please

SparkQA · 2019-08-27T12:16:54Z

Test build #109811 has finished for PR 25309 at commit b3b5f83.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2019-08-27T13:55:29Z

docs/configuration.md

-    (e.g. increase <code>spark.driver.memoryOverhead</code> or
-    <code>spark.executor.memoryOverhead</code>).
+    <em>Note:</em> If off-heap memory is enabled, may need to raise 
+    <code>spark.driver.memoryOverhead</code> size.


actually this brings up a good question, the size configs say they work for off heap size for executors, so what cases does this need to apply to the driver. Is this config really applying to both driver and executors for things like broadcast blocks, etc.

I have the same question :)，broadcast in driver side may not use offheap, TorrentBroadcast.writeBlocks method call blockManager.putSingle method use StorageLevel.MEMORY_AND_DISK and call blockManager.putBytes method use StorageLevel.MEMORY_AND_DISK_SER, both these 2 StorageLevel not use offheap memory, maybe we should remove these 2 line.

I found @beliefer add these description in #24671, can you help us to explain where offheap is used in driver side? Thx ~

Originally, off-heap memory will not affect the container memory, so we must consider it and config spark.driver.memoryOverhead bigger.

But which component uses offheap in driver side?

beliefer · 2019-08-28T02:35:56Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala

  private[yarn] val resource: Resource = {
    val resource = Resource.newInstance(
-      executorMemory + memoryOverhead + pysparkWorkerMemory, executorCores)
+      executorMemory + executorOffHeapMemory + memoryOverhead + pysparkWorkerMemory, executorCores)


According line 258 to 260 in docs/configuration.md, memoryOverhead includes pysparkWorkerMemory, but looks difference here.

Described in the document is Additional memory includes PySpark executor memory (when <code>spark.executor.pyspark.memory</code> is not configured), I think we need a new jira to discuss how to solve this problem.

Yes, this makes me confused.

Isn't this what spark.executor.memoryOverhead already adds via memoryOverhead?
I guess I'm wondering what MEMORY_OFFHEAP_SIZE does that's supposed to be different.

@srowen memoryOverhead include MEMORY_OFFHEAP_SIZE before this pr, and memoryOverhead and MEMORY_OFFHEAP_SIZE had to be modified at the same time to ensure request enough resources from yarn if I want to increase MEMORY_OFFHEAP_SIZE , this is not user friendly, and this has always been confusing, why we need to modify two memory-related parameters simultaneously for one purpose? This pr let them be independent.

beliefer · 2019-08-28T09:58:34Z

docs/configuration.md

-    processes running in the same container. The maximum memory size of container to running executor 
-    is determined by the sum of <code>spark.executor.memoryOverhead</code> and 
-    <code>spark.executor.memory</code>.
+    <em>Note:</em> Additional memory includes PySpark executor memory 


another problem is Additional better than Non-heap?

How to explain Non-heap? Now MemoryOverHead not includes offheap, and maybe PySpark executor memory should add a default value and separate from MemoryOverHead, should we redefinition this concept.
Any other suggested names?

So this PR looks a little conflict with origin definition.
Non-heap includes all the memory but java heap.

that is the point, we are changing it so that you don't have to include off heap inside of overhead memory. User is already specifying off heap size so why should they have to add it to overhead memory? it works just the other configs - pyspark memory, heap memory,

I know the meaning of this PR. Maybe the new idea is a way. As I know, the origin decision is to unify all the different part.

LuciferYang · 2019-08-28T11:19:37Z

docs/configuration.md

  <td>executorMemory * 0.10, with minimum of 384 </td>
  <td>
-    Amount of non-heap memory to be allocated per executor process in cluster mode, in MiB unless
+    Amount of additional memory to be allocated per executor process in cluster mode, in MiB unless


maybe we should remove interned strings, it use heap space after Java8

SparkQA · 2019-09-04T03:55:56Z

Test build #110085 has finished for PR 25309 at commit bb29488.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs

+1

xuanyuanking · 2019-09-05T03:54:11Z

@LuciferYang Because it has been merged to master by Thomas. :)
Thanks for your contribution.

LuciferYang · 2019-09-05T05:35:31Z

Thanks to all reviewers ~ @jerryshao @tgravescs @xuanyuanking @kiszk @beliefer @srowen

LuciferYang added 2 commits July 31, 2019 14:23

[SPARK-28577]Ensure executorMemoryHead requested value not less than …

13b81d2

…offHeapSize when offHeap is enable

fix-import

ff9e2e4

LuciferYang changed the title ~~[SPARK-28577]Ensure executorMemoryHead requested value not less than offHeapSize when offHeap is enabl~~ [SPARK-28577]Ensure executorMemoryOverHead requested value not less than offHeapSize when offHeap is enable Jul 31, 2019

fix-typo

cd27192

LuciferYang changed the title ~~[SPARK-28577]Ensure executorMemoryOverHead requested value not less than offHeapSize when offHeap is enable~~ [SPARK-28577]Ensure executorMemoryOverHead requested value not less than offHeapSize when offHeap enable Jul 31, 2019

LuciferYang changed the title ~~[SPARK-28577]Ensure executorMemoryOverHead requested value not less than offHeapSize when offHeap enable~~ [SPARK-28577][YARN]Ensure executorMemoryOverHead requested value not less than offHeapSize when offHeap enable Jul 31, 2019

dongjoon-hyun added the YARN label Jul 31, 2019

kiszk reviewed Jul 31, 2019

View reviewed changes

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala Outdated Show resolved Hide resolved

kiszk reviewed Jul 31, 2019

View reviewed changes

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala Outdated Show resolved Hide resolved

use config.key instead of plan text

c44a33e

xuanyuanking reviewed Aug 2, 2019

View reviewed changes

only print a warn log when offheap more than overhead

9421fbe

jerryshao reviewed Aug 2, 2019

View reviewed changes

LuciferYang added 2 commits August 2, 2019 23:05

Add spark.memory.offHeap.size to request resource

c0ef860

Add spark.memory.offHeap.size to request resource

4fb5362

LuciferYang changed the title ~~[SPARK-28577][YARN]Ensure executorMemoryOverHead requested value not less than offHeapSize when offHeap enable~~ [SPARK-28577][YARN]Resource capability requested for each executor add offHeapSize Aug 2, 2019

LuciferYang changed the title ~~[SPARK-28577][YARN]Resource capability requested for each executor add offHeapSize~~ [SPARK-28577][YARN]Resource capability requested for each executor add offHeapMemorySize Aug 2, 2019

jerryshao reviewed Aug 8, 2019

View reviewed changes

LuciferYang added 2 commits August 8, 2019 19:36

add a space after +

66ae9a1

Add a test case to check memory resource include offheap part

c03db87

tgravescs reviewed Aug 23, 2019

View reviewed changes

Use Utils.memoryStringToMb to convert offheap size

0020a02

jerryshao reviewed Aug 26, 2019

View reviewed changes

remove unnecessary string interpolation

02c0f2a

tgravescs approved these changes Aug 26, 2019

View reviewed changes

update doc

b3b5f83

tgravescs reviewed Aug 27, 2019

View reviewed changes

beliefer reviewed Aug 28, 2019

View reviewed changes

LuciferYang commented Aug 28, 2019

View reviewed changes

update doc

bb29488

tgravescs approved these changes Sep 4, 2019

View reviewed changes

asfgit closed this in a07f795 Sep 4, 2019

LuciferYang deleted the spark-28577 branch June 6, 2022 03:42

[SPARK-28577][YARN]Resource capability requested for each executor add offHeapMemorySize #25309

[SPARK-28577][YARN]Resource capability requested for each executor add offHeapMemorySize #25309

Uh oh!

Conversation

LuciferYang commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

LuciferYang commented Jul 31, 2019

Uh oh!

Uh oh!

Uh oh!

LuciferYang commented Jul 31, 2019

Uh oh!

xuanyuanking left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryshao Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryshao commented Aug 6, 2019

Uh oh!

SparkQA commented Aug 6, 2019

Uh oh!

beliefer commented Aug 7, 2019

Uh oh!

jerryshao commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Jul 31, 2019 •

edited

Loading

LuciferYang Aug 2, 2019 •

edited

Loading

jerryshao Aug 2, 2019 •

edited

Loading

LuciferYang Aug 2, 2019 •

edited

Loading

LuciferYang Aug 6, 2019 •

edited

Loading

beliefer Aug 6, 2019 •

edited

Loading

LuciferYang Aug 8, 2019 •

edited

Loading

LuciferYang Aug 8, 2019 •

edited

Loading

LuciferYang Aug 23, 2019 •

edited

Loading

LuciferYang Aug 26, 2019 •

edited

Loading

LuciferYang Aug 27, 2019 •

edited

Loading