You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/01/04 09:26:01 UTC
[GitHub] [spark] LuciferYang opened a new pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
LuciferYang opened a new pull request #35095:
URL: https://github.com/apache/spark/pull/35095
### What changes were proposed in this pull request?
`SPARK-37795` Add a scalastyle rule to ban `org.apache.log4j` imports, but there is still one place to retain the imports of `org.apache.log4j` in `o.a.spark.TestUtils`.
This pr refactor `configTestLog4j` method in `o.a.spark.TestUtils` to use log4j2 api and rename the method name from `configTestLog4j` to `configTestLog4j2`.
### Why are the changes needed?
Clean up the `org.apache.log4j` imports left in Spark internal.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Pass the Jenkins or GitHub Action
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778517920
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
If we all agree to remove `configTestLog4j `, I will update the pr and pr description
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778515785
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
I traced back to the original pr ([SPARK-3193](https://github.com/apache/spark/pull/2108)), it seems that `configTestLog4j` is only added to help troubleshoot the failed UT on Jenkins.
~~I agree to remove this method directly,~~ what do you think @dongjoon-hyun ?
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778517920
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
If we are sure to remove the `configTestLog4j `directly, I will update the pr and pr description
--
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
[GitHub] [spark] LuciferYang commented on pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35095:
URL: https://github.com/apache/spark/pull/35095#issuecomment-1004690356
> FYI, @LuciferYang . It was intentionally ignored at that time after discussing with @williamhyun offline.
What is the specific reason? I didn't find it in SPARK-37795
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778515785
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
I traced back to the original pr ([SPARK-3193](https://github.com/apache/spark/pull/2108)) it seems that `configTestLog4j ` is only added to help troubleshoot the failed UT on Jenkins.
I agree to remove this method directly, what do you think @dongjoon-hyun ?
--
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
[GitHub] [spark] viirya commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r779276773
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
Review comment:
I'm not sure about https://github.com/apache/spark/pull/35095#issuecomment-1004685780.
--
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
[GitHub] [spark] LuciferYang commented on pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35095:
URL: https://github.com/apache/spark/pull/35095#issuecomment-1004672180
cc @dongjoon-hyun
--
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
[GitHub] [spark] viirya commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778531494
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
Then it is okay to keep it, I think.
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778517920
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
If we all agree to remove `configTestLog4j `, I will update the pr and pr description
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778515785
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
I traced back to the original pr ([SPARK-3193](https://github.com/apache/spark/pull/2108)), it seems that `configTestLog4j` is only added to help troubleshoot the failed UT on Jenkins about `the Process exitcode != 0`
~~I agree to remove this method directly,~~（EDIT） what do you think @dongjoon-hyun ?
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778515785
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
I traced back to the original pr ([SPARK-3193](https://github.com/apache/spark/pull/2108)), it seems that `configTestLog4j ` is only added to help troubleshoot the failed UT on Jenkins.
I agree to remove this method directly, what do you think @dongjoon-hyun ?
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
I traced back to the original pr ([SPARK-3193](https://github.com/apache/spark/pull/2108)), it seems that `configTestLog4j` is only added to help troubleshoot the failed UT on Jenkins.
I agree to remove this method directly, what do you think @dongjoon-hyun ?
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35095:
URL: https://github.com/apache/spark/pull/35095#issuecomment-1004685780
FYI, @LuciferYang . It was intentionally ignored at that time after discussing with @williamhyun offline.
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778515785
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
I traced back to the original pr ([SPARK-3193](https://github.com/apache/spark/pull/2108)), it seems that `configTestLog4j` is only added to help troubleshoot the failed UT on Jenkins.
~~I agree to remove this method directly,~~（EDIT） what do you think @dongjoon-hyun ?
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r779276399
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
Review comment:
@viirya Any other need change?
--
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
[GitHub] [spark] srowen commented on pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35095:
URL: https://github.com/apache/spark/pull/35095#issuecomment-1011566516
Merged to master
--
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
[GitHub] [spark] LuciferYang commented on pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35095:
URL: https://github.com/apache/spark/pull/35095#issuecomment-1010568147
> Looks OK pending tests
GA passed
--
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
[GitHub] [spark] LuciferYang commented on pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35095:
URL: https://github.com/apache/spark/pull/35095#issuecomment-1004736169
GA run failed，Let me check it first
--
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
[GitHub] [spark] viirya commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778581202
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
Review comment:
`s"$level"` -> `level`. `level` is already a string, no need to interpolate it.
--
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
[GitHub] [spark] LuciferYang commented on pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35095:
URL: https://github.com/apache/spark/pull/35095#issuecomment-1004832360
GA Passed
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778584155
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
Review comment:
cc06076 fix this
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778531612
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
ok ~
--
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
[GitHub] [spark] viirya commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778297702
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
Maybe we can just remove `configTestLog4j`?
--
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
[GitHub] [spark] srowen closed pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #35095:
URL: https://github.com/apache/spark/pull/35095
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35095:
URL: https://github.com/apache/spark/pull/35095#issuecomment-1004684985
cc @viirya
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778523992
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
Hmm... It seems that the method of calling `configTestLog4j` in the current UTs is in the process of `runSparkSubmit`. If we directly remove this method, I can not find out where to see the relevant log of `runSparkSubmit` (not in `target/units`), which is really not conducive to troubleshooting.
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r779277107
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
Review comment:
thank @viirya , ping @dongjoon-hyun @williamhyun
--
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
[GitHub] [spark] LuciferYang commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778523992
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
Hmm... It seems that the method of calling `configTestLog4j` in the current UTs is in the process of `runSparkSubmit`. If we directly remove this method, I can not find out where to see the relevant log of `runSparkSubmit` (not in `target/unit-tests.log`), which is really not conducive to troubleshooting.
--
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
[GitHub] [spark] LuciferYang commented on pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35095:
URL: https://github.com/apache/spark/pull/35095#issuecomment-1011720332
thanks all
--
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
[GitHub] [spark] viirya commented on a change in pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35095:
URL: https://github.com/apache/spark/pull/35095#discussion_r778297036
##########
File path: core/src/main/scala/org/apache/spark/TestUtils.scala
##########
@@ -418,17 +419,18 @@ private[spark] object TestUtils {
}
/**
- * config a log4j properties used for testsuite
+ * config a log4j2 properties used for testsuite
*/
- def configTestLog4j(level: String): Unit = {
- val pro = new Properties()
- pro.put("log4j.rootLogger", s"$level, console")
- pro.put("log4j.appender.console", "org.apache.log4j.ConsoleAppender")
- pro.put("log4j.appender.console.target", "System.err")
- pro.put("log4j.appender.console.layout", "org.apache.log4j.PatternLayout")
- pro.put("log4j.appender.console.layout.ConversionPattern",
- "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n")
- PropertyConfigurator.configure(pro)
+ def configTestLog4j2(level: String): Unit = {
+ val builder = ConfigurationBuilderFactory.newConfigurationBuilder()
+ val appenderBuilder = builder.newAppender("console", "CONSOLE")
+ .addAttribute("target", ConsoleAppender.Target.SYSTEM_ERR)
+ appenderBuilder.add(builder.newLayout("PatternLayout")
+ .addAttribute("pattern", "%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n"))
+ builder.add(appenderBuilder)
+ builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+ val configuration = builder.build()
+ LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)
Review comment:
This rewriting looks okay. I just wonder the purpose of `configTestLog4j` and if it is necessary. These tests are not checking the log (as they are still passed with no-op `configTestLog4j` now). And seems it is not used to reduce verbose logging.
--
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
[GitHub] [spark] LuciferYang commented on pull request #35095: [SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35095:
URL: https://github.com/apache/spark/pull/35095#issuecomment-1010090051
8d0a078 merge with master
--
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