Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
yaooqinn wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f5b67a
limit the size for am to unregister itself from rm
yaooqinn Aug 22, 2018
3271c3f
nit
yaooqinn Aug 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@xuanyuanking xuanyuanking Aug 22, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move this to L165? just for code clean.

@vanzin vanzin Aug 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to just read the value at the point where it's needed, given there's only one use.

@volatile private var userClassThread: Thread = _

@volatile private var reporterThread: Thread = _
Expand Down Expand Up @@ -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) {

@vanzin vanzin Aug 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringUtils.abbreviate seems simpler.

@yaooqinn yaooqinn Aug 23, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's better, thanks

msg
} else {
msg.substring(0, finalMsgLimitSize)

@xuanyuanking xuanyuanking Aug 22, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the message in last finalMsgLimitSize is more useful.

}
finished = true
if (!inShutdown && Thread.currentThread() != reporterThread && reporterThread != null) {
logDebug("shutting down reporter thread")
Expand Down
6 changes: 6 additions & 0 deletions resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/config.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")

@HyukjinKwon HyukjinKwon Aug 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for leaving a comment late, and nitpicking but shouldn't we better leave this unlimited by default?

@yaooqinn yaooqinn Aug 25, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/* Client-mode AM configuration. */

private[spark] val AM_CORES = ConfigBuilder("spark.yarn.am.cores")
Expand Down