Skip to content
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
19 changes: 15 additions & 4 deletions common/kvstore/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,20 @@
<artifactId>commons-io</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>log4j</groupId>
<artifactId>log4j</artifactId>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand All @@ -77,8 +88,8 @@
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
25 changes: 18 additions & 7 deletions common/network-common/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,26 @@

<!-- Test dependencies -->
<dependency>
<groupId>log4j</groupId>
<artifactId>log4j</artifactId>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.apache.spark</groupId>
<artifactId>spark-tags_${scala.binary.version}</artifactId>
Expand All @@ -128,11 +144,6 @@
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
20 changes: 18 additions & 2 deletions common/network-shuffle/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,26 @@
</dependency>

<dependency>
<groupId>log4j</groupId>
<artifactId>log4j</artifactId>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
Expand Down
18 changes: 14 additions & 4 deletions core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>

<dependency>
<groupId>org.slf4j</groupId>
<artifactId>jul-to-slf4j</artifactId>
Expand All @@ -213,13 +214,22 @@
<artifactId>jcl-over-slf4j</artifactId>
</dependency>
<dependency>
<groupId>log4j</groupId>
<artifactId>log4j</artifactId>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</artifactId>
</dependency>

<dependency>
<groupId>com.ning</groupId>
<artifactId>compress-lzf</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#


# Set everything to be logged to the console
appender.console.type = Console
appender.console.name = STDOUT
appender.console.target = SYSTEM_OUT

@Kimahriman Kimahriman Sep 11, 2022

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.

Random thing I just started noticing, the default logging now goes to stdout instead of stderr, was that intentional? The log4j2.properties.template uses stderr too.

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.

# Set everything to be logged to the console
log4j.rootCategory=INFO, console
log4j.appender.console=org.apache.log4j.ConsoleAppender
log4j.appender.console.target=System.err
log4j.appender.console.layout=org.apache.log4j.PatternLayout
log4j.appender.console.layout.ConversionPattern=%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n

rootLogger.level = info
rootLogger.appenderRef.stdout.ref = STDOUT
appender.console.type = Console
appender.console.name = STDOUT
appender.console.target = SYSTEM_OUT
appender.console.layout.type = PatternLayout
appender.console.layout.pattern = %d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n%ex

look different, WDYT? @viirya

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.

Might be when I copied across properties file and missed to change.

@LuciferYang LuciferYang Sep 12, 2022

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.

roughly scanned, seems only this place changes from stderr to stdout, #37854 try fix this

appender.console.layout.type = PatternLayout
appender.console.layout.pattern = %d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n

# Settings to quiet third party logs that are too verbose
logger.jetty.name = org.sparkproject.jetty
logger.jetty.level = warn
logger.jetty2.name = org.sparkproject.jetty.util.component.AbstractLifeCycle
logger.jetty2.level = error
logger.repl1.name = org.apache.spark.repl.SparkIMain$exprTyper
logger.repl1.level = info
logger.repl2.name = org.apache.spark.repl.SparkILoop$SparkILoopInterpreter
logger.repl2.level = info

rootLogger.level = warn
rootLogger.appenderRef.stdout.ref = STDOUT

94 changes: 15 additions & 79 deletions core/src/main/scala/org/apache/spark/internal/Logging.scala
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,8 @@

package org.apache.spark.internal

import scala.collection.JavaConverters._

import org.apache.log4j._
import org.apache.log4j.spi.{Filter, LoggingEvent}
import org.apache.logging.log4j.LogManager
import org.apache.logging.log4j.core.LoggerContext
import org.slf4j.{Logger, LoggerFactory}
import org.slf4j.impl.StaticLoggerBinder

Expand Down Expand Up @@ -122,49 +120,19 @@ trait Logging {
}

private def initializeLogging(isInterpreter: Boolean, silent: Boolean): Unit = {
// Don't use a logger in here, as this is itself occurring during initialization of a logger
// If Log4j 1.2 is being used, but is not initialized, load a default properties file
if (Logging.isLog4j12()) {
val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
if (!Logging.isLog4j12()) {

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.

The code guarded by if(!Logging.isLog4j12()) only works with Log4J2 and not other SLF4J bindings (such as Logback). I think it might be a bit clearer and slightly more correct to replace this with a positive if(Logging.isLog4j2()) check which checks that the Log4J2 binding is in use (e.g. that the binder class is org.apache.logging.slf4j.Log4jLoggerFactory).

Similarly, I think log4j12Initialized should be renamed to log4j2Initialized (since we're not using Log4J 1.2 in the branch where that variable is defined).

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.

Agreed. I will fix them.

// scalastyle:off println
if (!log4j12Initialized) {
Logging.defaultSparkLog4jConfig = true
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
if (!silent) {
System.err.println(s"Using Spark's default log4j profile: $defaultLogProps")
}
case None =>
System.err.println(s"Spark was unable to load $defaultLogProps")
}
}

val rootLogger = LogManager.getRootLogger()
if (Logging.defaultRootLevel == null) {
Logging.defaultRootLevel = rootLogger.getLevel()
}

if (isInterpreter) {
// Use the repl's main class to define the default log level when running the shell,
// overriding the root logger's config if they're different.
val replLogger = LogManager.getLogger(logName)
val replLevel = Option(replLogger.getLevel()).getOrElse(Level.WARN)
// Update the consoleAppender threshold to replLevel
if (replLevel != rootLogger.getEffectiveLevel()) {
Logging.defaultSparkLog4jConfig = true
val defaultLogProps = "org/apache/spark/log4j2-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
val context = LogManager.getContext(false).asInstanceOf[LoggerContext]
context.setConfigLocation(url.toURI)
if (!silent) {
System.err.printf("Setting default log level to \"%s\".\n", replLevel)
System.err.println("To adjust logging level use sc.setLogLevel(newLevel). " +
"For SparkR, use setLogLevel(newLevel).")
}
Logging.sparkShellThresholdLevel = replLevel
rootLogger.getAllAppenders().asScala.foreach {
case ca: ConsoleAppender =>
ca.addFilter(new SparkShellLoggingFilter())
case _ => // no-op
System.err.println(s"Using Spark's default log4j profile: $defaultLogProps")
}
}
case None =>
System.err.println(s"Spark was unable to load $defaultLogProps")
}
// scalastyle:on println
}
Expand All @@ -178,9 +146,7 @@ trait Logging {

private[spark] object Logging {
@volatile private var initialized = false
@volatile private var defaultRootLevel: Level = null
@volatile private var defaultSparkLog4jConfig = false
@volatile private[spark] var sparkShellThresholdLevel: Level = null

val initLock = new Object()
try {
Expand All @@ -202,14 +168,11 @@ private[spark] object Logging {
* initialization again.
*/
def uninitialize(): Unit = initLock.synchronized {
if (isLog4j12()) {
if (!isLog4j12()) {
if (defaultSparkLog4jConfig) {
defaultSparkLog4jConfig = false
LogManager.resetConfiguration()
} else {
val rootLogger = LogManager.getRootLogger()
rootLogger.setLevel(defaultRootLevel)
sparkShellThresholdLevel = null
val context = LogManager.getContext(false).asInstanceOf[LoggerContext]
context.reconfigure()
}
}
this.initialized = false
Expand All @@ -223,30 +186,3 @@ private[spark] object Logging {
"org.slf4j.impl.Log4jLoggerFactory".equals(binderClass)
}
}

private class SparkShellLoggingFilter extends Filter {

/**
* If sparkShellThresholdLevel is not defined, this filter is a no-op.
* If log level of event is not equal to root level, the event is allowed. Otherwise,
* the decision is made based on whether the log came from root or some custom configuration
* @param loggingEvent
* @return decision for accept/deny log event
*/
def decide(loggingEvent: LoggingEvent): Int = {
if (Logging.sparkShellThresholdLevel == null) {
Filter.NEUTRAL
} else if (loggingEvent.getLevel.isGreaterOrEqual(Logging.sparkShellThresholdLevel)) {
Filter.NEUTRAL
} else {
var logger = loggingEvent.getLogger()
while (logger.getParent() != null) {
if (logger.getLevel != null || logger.getAllAppenders.hasMoreElements) {
return Filter.NEUTRAL
}
logger = logger.getParent()
}
Filter.DENY
}
}
}
2 changes: 0 additions & 2 deletions core/src/main/scala/org/apache/spark/util/Utils.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2426,8 +2426,6 @@ private[spark] object Utils extends Logging {
def setLogLevel(l: org.apache.log4j.Level): Unit = {
val rootLogger = org.apache.log4j.Logger.getRootLogger()
rootLogger.setLevel(l)
// Setting threshold to null as rootLevel will define log level for spark-shell
Logging.sparkShellThresholdLevel = null
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ import org.apache.hadoop.conf.Configuration
import org.apache.hadoop.fs.{FileSystem, FSDataOutputStream, Path}
import org.apache.hadoop.fs.permission.FsPermission
import org.apache.hadoop.hdfs.client.HdfsDataOutputStream
import org.apache.log4j.{FileAppender => Log4jFileAppender, _}
import org.apache.logging.log4j._
import org.apache.logging.log4j.core.Logger
import org.apache.logging.log4j.core.appender.{FileAppender => Log4jFileAppender}
import org.apache.logging.log4j.core.layout.PatternLayout

import org.apache.spark.SparkConf
import org.apache.spark.deploy.SparkHadoopUtil
Expand All @@ -51,17 +54,17 @@ private[spark] class DriverLogger(conf: SparkConf) extends Logging {
addLogAppender()

private def addLogAppender(): Unit = {
val appenders = LogManager.getRootLogger().getAllAppenders()
val logger = LogManager.getRootLogger().asInstanceOf[Logger]
val layout = if (conf.contains(DRIVER_LOG_LAYOUT)) {
new PatternLayout(conf.get(DRIVER_LOG_LAYOUT).get)
} else if (appenders.hasMoreElements()) {
appenders.nextElement().asInstanceOf[Appender].getLayout()
PatternLayout.newBuilder().withPattern(conf.get(DRIVER_LOG_LAYOUT).get).build()
} else {
new PatternLayout(DEFAULT_LAYOUT)
PatternLayout.newBuilder().withPattern(conf.get(DEFAULT_LAYOUT)).build()
}
val fa = new Log4jFileAppender(layout, localLogFile)
fa.setName(DriverLogger.APPENDER_NAME)
LogManager.getRootLogger().addAppender(fa)
val config = logger.getContext.getConfiguration()
val fa = Log4jFileAppender.createAppender(localLogFile, "false", "false",
DriverLogger.APPENDER_NAME, "true", "false", "false", "4000", layout, null,
"false", null, config);
Comment on lines +64 to +66

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.

My IDE says that this createAppender method is deprecated. Since the old 1. code was only setting the filename and layout, could this instead be

    val fa = Log4jFileAppender.newBuilder().withFileName(localLogFile).setLayout(layout).build()

or do we need to set some of those other arguments?

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.

I think we can rewrite it using builder.

logger.addAppender(fa)
logInfo(s"Added a local log appender at: ${localLogFile}")
}

Expand All @@ -78,9 +81,10 @@ private[spark] class DriverLogger(conf: SparkConf) extends Logging {

def stop(): Unit = {
try {
val fa = LogManager.getRootLogger.getAppender(DriverLogger.APPENDER_NAME)
LogManager.getRootLogger().removeAppender(DriverLogger.APPENDER_NAME)
Utils.tryLogNonFatalError(fa.close())
val logger = LogManager.getRootLogger().asInstanceOf[Logger]
val fa = logger.getAppenders.get(DriverLogger.APPENDER_NAME)
logger.removeAppender(fa)
Utils.tryLogNonFatalError(fa.stop())
writer.foreach(_.closeWriter())
} catch {
case e: Exception =>
Expand Down
Loading