You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by yaooqinn <gi...@git.apache.org> on 2018/08/22 02:24:05 UTC

[GitHub] spark pull request #22180: [SPARK-25174][YARN]Limit the size of diagnostic m...

GitHub user yaooqinn opened a pull request:

    https://github.com/apache/spark/pull/22180

    [SPARK-25174][YARN]Limit the size of diagnostic message for am to unregister itself from rm

    ## What changes were proposed in this pull request?
    
    When using older versions of spark releases,  a use case generated a huge code-gen file which hit the limitation `Constant pool has grown past JVM limit of 0xFFFF`.  In this situation, it should fail immediately. But the diagnosis message sent to RM is too large,  the ApplicationMaster suspended and RM's ZKStateStore was crashed. For 2.3 or later spark releases the limitation of code-gen has been removed, but maybe there are still some uncaught exceptions that contain oversized error message will cause such a problem.
    
    This PR is aim to cut down the diagnosis message size.
    
    ## How was this patch tested?
    
    Please review http://spark.apache.org/contributing.html before opening a pull request.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/yaooqinn/spark SPARK-25174

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/22180.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #22180
    
----
commit 8f5b67a57f6f8e9237fbfcfd9f80a02ee73cfe5d
Author: Kent Yao <ya...@...>
Date:   2018-08-22T02:01:28Z

    limit the size for am to unregister itself from rm

----


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2413/
    Test PASSed.


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2468/
    Test PASSed.


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by yaooqinn <gi...@git.apache.org>.
Github user yaooqinn commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    cc @gatorsmile @vanzin 


---

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

[GitHub] spark pull request #22180: [SPARK-25174][YARN]Limit the size of diagnostic m...

Posted by yaooqinn <gi...@git.apache.org>.
Github user yaooqinn commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22180#discussion_r212790276
  
    --- Diff: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/config.scala ---
    @@ -192,6 +192,12 @@ package object config {
         .toSequence
         .createWithDefault(Nil)
     
    +  private[spark] val AM_FINAL_MSG_LIMIT = ConfigBuilder("spark.yarn.am.finalMessageLimit")
    +    .doc("The limit size of final diagnostic message for our ApplicationMaster to unregister from" +
    +      " the ResourceManager.")
    +    .bytesConf(ByteUnit.BYTE)
    +    .createWithDefaultString("1m")
    --- End diff --
    
    in https://issues.apache.org/jira/browse/YARN-6125 `yarn.app.attempt.diagnostics.limit.kc` 's default value is `64K`. I guess `1m` here is enough


---

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

[GitHub] spark pull request #22180: [SPARK-25174][YARN]Limit the size of diagnostic m...

Posted by xuanyuanking <gi...@git.apache.org>.
Github user xuanyuanking commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22180#discussion_r211996874
  
    --- Diff: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala ---
    @@ -143,6 +143,7 @@ private[spark] class ApplicationMaster(args: ApplicationMasterArguments) extends
       @volatile private var finished = false
       @volatile private var finalStatus = getDefaultFinalStatus
       @volatile private var finalMsg: String = ""
    +  private val finalMsgLimitSize = sparkConf.get(AM_FINAL_MSG_LIMIT).toInt
    --- End diff --
    
    nit: move this to L165? just for code clean.


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    Looks good, merging to master.


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    **[Test build #95073 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95073/testReport)** for PR 22180 at commit [`8f5b67a`](https://github.com/apache/spark/commit/8f5b67a57f6f8e9237fbfcfd9f80a02ee73cfe5d).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    Merged build finished. Test PASSed.


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95132/
    Test PASSed.


---

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

[GitHub] spark pull request #22180: [SPARK-25174][YARN]Limit the size of diagnostic m...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22180#discussion_r212059380
  
    --- Diff: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala ---
    @@ -368,7 +369,11 @@ private[spark] class ApplicationMaster(args: ApplicationMasterArguments) extends
             }
             logInfo(s"Final app status: $finalStatus, exitCode: $exitCode" +
               Option(msg).map(msg => s", (reason: $msg)").getOrElse(""))
    -        finalMsg = msg
    +        finalMsg = if (msg == null || msg.length <= finalMsgLimitSize) {
    --- End diff --
    
    `StringUtils.abbreviate` seems simpler.


---

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

[GitHub] spark pull request #22180: [SPARK-25174][YARN]Limit the size of diagnostic m...

Posted by yaooqinn <gi...@git.apache.org>.
Github user yaooqinn commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22180#discussion_r212161599
  
    --- Diff: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala ---
    @@ -368,7 +369,11 @@ private[spark] class ApplicationMaster(args: ApplicationMasterArguments) extends
             }
             logInfo(s"Final app status: $finalStatus, exitCode: $exitCode" +
               Option(msg).map(msg => s", (reason: $msg)").getOrElse(""))
    -        finalMsg = msg
    +        finalMsg = if (msg == null || msg.length <= finalMsgLimitSize) {
    --- End diff --
    
    that's better, thanks


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    Merged build finished. Test PASSed.


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    **[Test build #95132 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95132/testReport)** for PR 22180 at commit [`3271c3f`](https://github.com/apache/spark/commit/3271c3f4100e0d69fe30400a42ab35aaab1c7c48).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    **[Test build #95132 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95132/testReport)** for PR 22180 at commit [`3271c3f`](https://github.com/apache/spark/commit/3271c3f4100e0d69fe30400a42ab35aaab1c7c48).


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    Merged build finished. Test PASSed.


---

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

[GitHub] spark pull request #22180: [SPARK-25174][YARN]Limit the size of diagnostic m...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22180#discussion_r212059286
  
    --- Diff: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala ---
    @@ -143,6 +143,7 @@ private[spark] class ApplicationMaster(args: ApplicationMasterArguments) extends
       @volatile private var finished = false
       @volatile private var finalStatus = getDefaultFinalStatus
       @volatile private var finalMsg: String = ""
    +  private val finalMsgLimitSize = sparkConf.get(AM_FINAL_MSG_LIMIT).toInt
    --- End diff --
    
    Better to just read the value at the point where it's needed, given there's only one use.


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95073/
    Test PASSed.


---

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

[GitHub] spark pull request #22180: [SPARK-25174][YARN]Limit the size of diagnostic m...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22180#discussion_r212790082
  
    --- Diff: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/config.scala ---
    @@ -192,6 +192,12 @@ package object config {
         .toSequence
         .createWithDefault(Nil)
     
    +  private[spark] val AM_FINAL_MSG_LIMIT = ConfigBuilder("spark.yarn.am.finalMessageLimit")
    +    .doc("The limit size of final diagnostic message for our ApplicationMaster to unregister from" +
    +      " the ResourceManager.")
    +    .bytesConf(ByteUnit.BYTE)
    +    .createWithDefaultString("1m")
    --- End diff --
    
    Sorry for leaving a comment late, and nitpicking but shouldn't we better leave this unlimited by default?


---

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

[GitHub] spark pull request #22180: [SPARK-25174][YARN]Limit the size of diagnostic m...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/spark/pull/22180


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    **[Test build #95073 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95073/testReport)** for PR 22180 at commit [`8f5b67a`](https://github.com/apache/spark/commit/8f5b67a57f6f8e9237fbfcfd9f80a02ee73cfe5d).


---

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

[GitHub] spark issue #22180: [SPARK-25174][YARN]Limit the size of diagnostic message ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22180
  
    Merged build finished. Test PASSed.


---

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

[GitHub] spark pull request #22180: [SPARK-25174][YARN]Limit the size of diagnostic m...

Posted by xuanyuanking <gi...@git.apache.org>.
Github user xuanyuanking commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22180#discussion_r211996461
  
    --- Diff: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala ---
    @@ -368,7 +369,11 @@ private[spark] class ApplicationMaster(args: ApplicationMasterArguments) extends
             }
             logInfo(s"Final app status: $finalStatus, exitCode: $exitCode" +
               Option(msg).map(msg => s", (reason: $msg)").getOrElse(""))
    -        finalMsg = msg
    +        finalMsg = if (msg == null || msg.length <= finalMsgLimitSize) {
    +          msg
    +        } else {
    +          msg.substring(0, finalMsgLimitSize)
    --- End diff --
    
    Maybe the message in last `finalMsgLimitSize` is more useful.


---

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