Skip to content

SPARK-1902 Silence stacktrace from logs when doing port failover to port n+1 #1019

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
wants to merge 3 commits into from
Closed

SPARK-1902 Silence stacktrace from logs when doing port failover to port n+1 #1019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d85eed
SPARK-1902 Silence stacktrace from logs when doing port failover to p…
ash211 Jun 9, 2014
1e2866b
Address CR comments
ash211 Jun 19, 2014
0dd02f7
Leave old org.eclipse.jetty silencing in place
ash211 Jun 19, 2014
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
1 change: 1 addition & 0 deletions conf/log4j.properties.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ log4j.appender.console.layout.ConversionPattern=%d{yy/MM/dd HH:mm:ss} %p %c{1}:

# Settings to quiet third party logs that are too verbose
log4j.logger.org.eclipse.jetty=WARN
Copy link
Contributor

@pwendell pwendell Jun 18, 2014

Choose a reason for hiding this comment

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

Shouldn't we keep this line as well? Otherwise you might also be changing the log level of other jetty.XX loggers

Copy link
Contributor Author

@ash211 ash211 Jun 19, 2014

Choose a reason for hiding this comment

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

I was assuming that this line was a prior failed attempt at silencing this exact issue that stuck around, so intentionally changed this logging.

My change logs more verbosely for everything under org.eclipse.jetty that isn't org.eclipse.jetty.util.component.AbstractLifeCycle since it's lowering the threshold from WARN down to level of the root, which is INFO. Even if we were silencing something from jetty in addition to the message this change addresses, it'd be better to target that class more directly rather than everything inside org.eclipse.jetty

Copy link
Contributor

@pwendell pwendell Jun 19, 2014

Choose a reason for hiding this comment

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

That other line has been in spark since way before we added this port retrying, so I don't think that's why it's there. IIRC there are various jetty services that will spit out logging information, especially at start up and shut down, and so we've always used this blanket statement. See for example Spark 0.5 which is more than 2 years old (before we even had a UI).

https://github.com/apache/spark/blob/branch-0.5/conf/log4j.properties.template#L7

If you'd like to audit this at some point that would be great (it might be hard to find all cases where jetty will print something). But since this change is about the port issue specifically, it might be good not to change that other behavior in this patch

Copy link
Contributor Author

@ash211 ash211 Jun 19, 2014

Choose a reason for hiding this comment

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

Hmm, an audit of jetty logging doesn't sound particularly appealing TBH :) I think I'll leave it in instead

log4j.logger.org.eclipse.jetty.util.component.AbstractLifeCycle=ERROR
log4j.logger.org.apache.spark.repl.SparkIMain$exprTyper=INFO
log4j.logger.org.apache.spark.repl.SparkILoop$SparkILoopInterpreter=INFO
1 change: 1 addition & 0 deletions core/src/main/resources/org/apache/spark/log4j-defaults.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ log4j.appender.console.layout.ConversionPattern=%d{yy/MM/dd HH:mm:ss} %p %c{1}:

# Settings to quiet third party logs that are too verbose
log4j.logger.org.eclipse.jetty=WARN
log4j.logger.org.eclipse.jetty.util.component.AbstractLifeCycle=ERROR
log4j.logger.org.apache.spark.repl.SparkIMain$exprTyper=INFO
log4j.logger.org.apache.spark.repl.SparkILoop$SparkILoopInterpreter=INFO
11 changes: 8 additions & 3 deletions core/src/main/scala/org/apache/spark/ui/JettyUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,16 @@ private[spark] object JettyUtils extends Logging {
case s: Success[_] =>
(server, server.getConnectors.head.getLocalPort)
case f: Failure[_] =>
val nextPort = (currentPort + 1) % 65536
server.stop()
pool.stop()
logInfo("Failed to create UI at port, %s. Trying again.".format(currentPort))
logInfo("Error was: " + f.toString)
connect((currentPort + 1) % 65536)
val msg = s"Failed to create UI on port :$currentPort. Trying again on port :$nextPort"
Copy link
Contributor

@pwendell pwendell Jun 19, 2014

Choose a reason for hiding this comment

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

The : is a place holder for the word "port" in URI's, I it might be sufficient to just say "port XX" instead of "port :XX"

if(f.toString.contains("Address already in use")) {
Copy link
Contributor

@pwendell pwendell Jun 19, 2014

Choose a reason for hiding this comment

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

This might fail our style checks, should be if (f.toString...

logWarning(s"$msg - $f")
} else {
logError(msg, f.exception)
}
connect(nextPort)
}
}

Expand Down