[GitHub] [spark] JoshRosen commented on a change in pull request #34895: [SPARK-6305][BUILD] Migrate from log4j1 to log4j2

[GitBox <gi...@apache.org>]

JoshRosen commented on a change in pull request #34895:
URL: https://github.com/apache/spark/pull/34895#discussion_r772728895



##########
File path: core/src/main/scala/org/apache/spark/internal/Logging.scala
##########
@@ -222,31 +231,119 @@ private[spark] object Logging {
     val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
     "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
+  private class SparkShellLoggingFilter extends Filter {

Review comment:
       I think we could potentially simplify this class by having it extend `AbstractFilter` instead of `Filter`:
   
   From AbstractFilter's Javadoc:
   
   ```java
   /**
    * Users should extend this class to implement filters. Filters can be either context wide or attached to
    * an appender. A filter may choose to support being called only from the context or only from an appender in
    * which case it will only implement the required method(s). The rest will default to return {@link org.apache.logging.log4j.core.Filter.Result#NEUTRAL}.
    * <p>
    * Garbage-free note: the methods with unrolled varargs by default delegate to the
    * {@link #filter(Logger, Level, Marker, String, Object...) filter method with vararg parameters}.
    * Subclasses that want to be garbage-free should override these methods to implement the appropriate filtering
    * without creating a vararg array.
    * </p>
    */
   public abstract class AbstractFilter extends AbstractLifeCycle implements Filter {
   ```

##########
File path: core/src/test/scala/org/apache/spark/internal/LoggingSuite.scala
##########
@@ -1,55 +0,0 @@
-/*
- * 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.
- */
-
-package org.apache.spark.internal
-
-import org.apache.log4j.{Level, Logger}
-import org.apache.log4j.spi.{Filter, LoggingEvent}
-
-import org.apache.spark.SparkFunSuite
-import org.apache.spark.util.Utils
-
-class LoggingSuite extends SparkFunSuite {

Review comment:
       Why did we need to delete this suite? I think this suite's removal means that we no longer have unit test coverage of `SparkShellLoggingFilter`?

##########
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##########
@@ -2423,9 +2423,11 @@ private[spark] object Utils extends Logging {
   /**
    * configure a new log4j level
    */
-  def setLogLevel(l: org.apache.log4j.Level): Unit = {
-    val rootLogger = org.apache.log4j.Logger.getRootLogger()
+  def setLogLevel(l: org.apache.logging.log4j.Level): Unit = {
+    val rootLogger = org.apache.logging.log4j.LogManager.getRootLogger()
+      .asInstanceOf[org.apache.logging.log4j.core.Logger]
     rootLogger.setLevel(l)
+    rootLogger.get().setLevel(l)

Review comment:
       Do we know if this is the best way to programmatically change the root logger's level? I noticed that https://stackoverflow.com/questions/23434252/programmatically-change-log-level-in-log4j2 mentions several techniques which differ from the one used here.
   
   I ask because I'm not very familiar with Log4J2 and I'm unsure if there's subtle pitfalls to avoid (e.g. will this work if we don't call `updateLoggers()`?).
   
   If we wind up changing this code then we should also update similar code in `SparkFunSuite`.

##########
File path: core/src/main/scala/org/apache/spark/internal/Logging.scala
##########
@@ -122,17 +125,18 @@ 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()) {

Review comment:
       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).

##########
File path: core/src/main/scala/org/apache/spark/util/logging/DriverLogger.scala
##########
@@ -51,17 +54,18 @@ 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(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);

Review comment:
       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?

##########
File path: core/src/main/scala/org/apache/spark/util/logging/DriverLogger.scala
##########
@@ -78,9 +82,11 @@ 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)
+      fa.stop()

Review comment:
       Should we omit this `fa.stop()` since we have a `Utils.tryLogNonFatalError(fa.stop())` on the following line?

##########
File path: core/src/test/scala/org/apache/spark/SparkFunSuite.scala
##########
@@ -228,44 +229,62 @@ abstract class SparkFunSuite
    * appender and restores the log level if necessary.
    */
   protected def withLogAppender(
-      appender: Appender,
+      appender: AbstractAppender,
       loggerNames: Seq[String] = Seq.empty,
       level: Option[Level] = None)(
       f: => Unit): Unit = {
     val loggers = if (loggerNames.nonEmpty) {
-      loggerNames.map(Logger.getLogger)
+      loggerNames.map(LogManager.getLogger)
     } else {
-      Seq(Logger.getRootLogger)
+      Seq(LogManager.getRootLogger)
+    }
+    if (loggers.size == 0) {
+      throw new SparkException(s"Cannot get any logger to add the appender")
     }
     val restoreLevels = loggers.map(_.getLevel)
     loggers.foreach { logger =>
-      logger.addAppender(appender)
-      if (level.isDefined) {
-        logger.setLevel(level.get)
+      logger match {
+        case logger: Logger =>
+          logger.addAppender(appender)
+          appender.start()
+          if (level.isDefined) {
+            logger.setLevel(level.get)
+            logger.get().setLevel(level.get)
+          }
+        case _ =>

Review comment:
       Since `loggers` is a `Seq[Logger]`, is this branch ever reachable? Unless I'm mistaken, I think think we'll always hit the `case logger: Logger` case.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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