[MINOR][YARN] Make memLimitExceededLogMessage more clean #23030

wangyum · 2018-11-14T08:07:07Z

What changes were proposed in this pull request?

Current memLimitExceededLogMessage:

It‘s not very clear, because physical memory exceeds but suggestion contains virtual memory config. This pr makes it more clear and replace deprecated config: spark.yarn.executor.memoryOverhead.

How was this patch tested?

manual tests

SparkQA · 2018-11-14T08:31:02Z

Test build #98814 has finished for PR 23030 at commit e7ff43d.

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

wangyum · 2018-11-14T08:53:46Z

cc @vanzin

srowen · 2018-11-14T15:39:03Z

Because there are just two calls to this method, and now they behave pretty differently based on one argument, how about just inlining both calls and the regexes to make this simpler?

SparkQA · 2018-11-15T07:31:41Z

Test build #98859 has finished for PR 23030 at commit 44767f8.

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

srowen · 2018-11-15T15:26:19Z

resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnAllocatorSuite.scala

-    val pmemMsg = memLimitExceededLogMessage(diagnostics, PMEM_EXCEEDED_PATTERN)
-    assert(vmemMsg.contains("5.8 GB of 4.2 GB virtual memory used."))
-    assert(pmemMsg.contains("2.1 MB of 2 GB physical memory used."))
+    def logMessage(pattern: Pattern): String = {


I don't feel strongly about it, but I could just remove this test too, and inline the regexes to simplify this. This was just testing the exact error message and I don't know how useful that is.

srowen · 2018-11-15T15:56:32Z

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

+            } else {
+              ""
+            }
+            val matcher = VMEM_EXCEEDED_PATTERN.matcher(completedContainer.getDiagnostics)


You can inline these patterns to further simplify.

SparkQA · 2018-11-15T16:12:33Z

Test build #98877 has finished for PR 23030 at commit 965a984.

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

vanzin · 2018-11-15T17:53:05Z

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

-            (true, memLimitExceededLogMessage(
-              completedContainer.getDiagnostics,
-              PMEM_EXCEEDED_PATTERN))
+            val suggestion = s"Consider boosting ${EXECUTOR_MEMORY_OVERHEAD.key}"


Why isn't this a suggestion in the vmem case too? It can help, even if it's a sledgehammer. Otherwise, you're basically telling the user in that case: "whatcha gonna do? it failed."

vanzin · 2018-11-15T17:53:24Z

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

-            (true, memLimitExceededLogMessage(
-              completedContainer.getDiagnostics,
-              VMEM_EXCEEDED_PATTERN))
+            val suggestion = if (conf.getBoolean(YarnConfiguration.NM_VMEM_CHECK_ENABLED,


Can you even get into this place if vmem checks are disabled?

Ah, and another option that can help here is yarn.nodemanager.vmem-pmem-ratio.

vanzin · 2018-11-15T17:56:24Z

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

+            val matcher = PMEM_EXCEEDED_PATTERN.matcher(completedContainer.getDiagnostics)
+            val diag = if (matcher.find()) " " + matcher.group() + "." else ""
+            val message =
+              s"Container killed by YARN for exceeding physical memory limits.$diag $suggestion."


Ignoring the above comment, suggestion is always set, so it could be inlined here.

Separately, it's always sub-optimal when formatting is scattered among different variables (e.g. the period at the end of the suggestion is here, the space before the diag is in the diag message itself).

SparkQA · 2018-11-16T09:03:18Z

Test build #98910 has finished for PR 23030 at commit 8ab6c3b.

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

srowen · 2018-11-16T14:55:34Z

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

-    Pattern.compile(s"$MEM_REGEX of $MEM_REGEX physical memory used")
-  val VMEM_EXCEEDED_PATTERN =
-    Pattern.compile(s"$MEM_REGEX of $MEM_REGEX virtual memory used")
+  val PMEM_EXCEEDED_PATTERN = raw"$MEM_REGEX of $MEM_REGEX physical memory used".r


We can still inline these patterns right?

Yes. I will do it.

SparkQA · 2018-11-16T15:29:33Z

Test build #98916 has finished for PR 23030 at commit 66b3e7f.

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

vanzin · 2018-11-16T18:28:03Z

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

+            val vmemExceededPattern = raw"$MEM_REGEX of $MEM_REGEX virtual memory used".r
+            val diag = vmemExceededPattern.findFirstIn(completedContainer.getDiagnostics)
+              .map(_.concat(".")).getOrElse("")
+            val additional = if (conf.getBoolean(YarnConfiguration.NM_VMEM_CHECK_ENABLED,


You didn't answer my question. How can you even get here if this config is disabled?

SparkQA · 2018-11-17T01:09:42Z

Test build #98944 has finished for PR 23030 at commit 9ad6df4.

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

vanzin · 2018-11-17T01:37:51Z

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

+              .map(_.concat(".")).getOrElse("")
+            val message = "Container killed by YARN for exceeding virtual memory limits. " +
+              s"$diag Consider boosting ${EXECUTOR_MEMORY_OVERHEAD.key} or disabling " +
+              s"${YarnConfiguration.NM_VMEM_CHECK_ENABLED} because of YARN-4714."


Now you removed the other config option I asked you to add... :-/

SparkQA · 2018-11-17T02:59:08Z

Test build #98946 has finished for PR 23030 at commit ca008cd.

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

srowen · 2018-11-20T14:28:03Z

Merged to master

## What changes were proposed in this pull request? Current `memLimitExceededLogMessage`: <img src="https://user-images.githubusercontent.com/5399861/48467789-ec8e1000-e824-11e8-91fc-280d342e1bf3.png" width="360"> It‘s not very clear, because physical memory exceeds but suggestion contains virtual memory config. This pr makes it more clear and replace deprecated config: ```spark.yarn.executor.memoryOverhead```. ## How was this patch tested? manual tests Closes apache#23030 from wangyum/EXECUTOR_MEMORY_OVERHEAD. Authored-by: Yuming Wang <yumwang@ebay.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

Make memLimitExceededLogMessage more clean

e7ff43d

remove memLimitExceededLogMessage

44767f8

srowen approved these changes Nov 15, 2018

View reviewed changes

remove test

965a984

srowen reviewed Nov 15, 2018

View reviewed changes

vanzin reviewed Nov 15, 2018

View reviewed changes

java -> scala makes it more simple

8ab6c3b

srowen reviewed Nov 16, 2018

View reviewed changes

inline patterns

66b3e7f

vanzin reviewed Nov 16, 2018

View reviewed changes

fix error

9ad6df4

vanzin reviewed Nov 17, 2018

View reviewed changes

Add NM_VMEM_PMEM_RATIO back

ca008cd

srowen approved these changes Nov 17, 2018

View reviewed changes

asfgit closed this in a00aaf6 Nov 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MINOR][YARN] Make memLimitExceededLogMessage more clean #23030

[MINOR][YARN] Make memLimitExceededLogMessage more clean #23030

wangyum commented Nov 14, 2018

SparkQA commented Nov 14, 2018

wangyum commented Nov 14, 2018

srowen commented Nov 14, 2018

SparkQA commented Nov 15, 2018

srowen Nov 15, 2018

srowen Nov 15, 2018

SparkQA commented Nov 15, 2018

vanzin Nov 15, 2018

vanzin Nov 15, 2018

vanzin Nov 15, 2018

vanzin Nov 15, 2018

SparkQA commented Nov 16, 2018

srowen Nov 16, 2018

wangyum Nov 16, 2018

SparkQA commented Nov 16, 2018

vanzin Nov 16, 2018

wangyum Nov 17, 2018

SparkQA commented Nov 17, 2018

vanzin Nov 17, 2018

SparkQA commented Nov 17, 2018

srowen commented Nov 20, 2018

[MINOR][YARN] Make memLimitExceededLogMessage more clean #23030

[MINOR][YARN] Make memLimitExceededLogMessage more clean #23030

Conversation

wangyum commented Nov 14, 2018

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Nov 14, 2018

wangyum commented Nov 14, 2018

srowen commented Nov 14, 2018

SparkQA commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 17, 2018

Choose a reason for hiding this comment

SparkQA commented Nov 17, 2018

srowen commented Nov 20, 2018