[SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api #35095

LuciferYang · 2022-01-04T09:25:53Z

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 let the log behavior using log4j2.x be the same as that using log4j1.x before. In fact, the configTestLog4j method behavior before this pr is invalid because PropertyConfigurator.configure method in org.apache.logging.log4j:log4j-1.2-api is an empty method as follows:

https://github.com/apache/logging-log4j2/blob/491a0b3787975b6fc95b6a8cb3da76dc7517c65f/log4j-1.2-api/src/main/java/org/apache/log4j/PropertyConfigurator.java#L39-L47

Another change of this pr is rename the method name from configTestLog4j to configTestLog4j2.

Why are the changes needed?

Clean up the org.apache.log4j imports left in Spark internal and let configTestLog4j method behavior keep consistent between log4j1.x and log4j2.x.

Does this PR introduce any user-facing change?

The configTestLog4j method in TestUtils rename to configTestLog4j2

How was this patch tested?

Pass the Jenkins or GitHub Action
Manual test

Run the test cases using configTestLog4j method in the following 3 scenarios:

without this pr to test log4j2.x
with this pr to test log4j2.x
run git reset --hard 19227983e91a54b5f27ade6412dad1088dfcba9e to test log4j1.x

For example WholeStageCodegenSparkSubmitSuite, run

mvn clean install -DskipTests -pl sql/core -am
mvn test -pl sql/core -Dtest=none -DwildcardSuites=org.apache.spark.sql.execution.WholeStageCodegenSparkSubmitSuite

Scenario 1 does not print any logs to the console, scenario 2 and scenario 3 will print similar logs to the console

LuciferYang · 2022-01-04T10:02:42Z

cc @dongjoon-hyun

dongjoon-hyun · 2022-01-04T10:19:22Z

cc @viirya

dongjoon-hyun · 2022-01-04T10:20:30Z

FYI, @LuciferYang . It was intentionally ignored at that time after discussing with @williamhyun offline.

LuciferYang · 2022-01-04T10:26:24Z

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

LuciferYang · 2022-01-04T11:37:54Z

GA run failed，Let me check it first

LuciferYang · 2022-01-04T13:56:59Z

GA Passed

viirya · 2022-01-04T18:24:46Z

core/src/main/scala/org/apache/spark/TestUtils.scala

+    builder.add(appenderBuilder)
+    builder.add(builder.newRootLogger(s"$level").add(builder.newAppenderRef("console")))
+    val configuration = builder.build()
+    LogManager.getContext(false).asInstanceOf[LoggerContext].reconfigure(configuration)


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.

Maybe we can just remove configTestLog4j?

I traced back to the original pr (SPARK-3193), 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 ?

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.

Then it is okay to keep it, I think.

viirya · 2022-01-05T06:49:52Z

core/src/main/scala/org/apache/spark/TestUtils.scala

+    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")))


s"$level" -> level. level is already a string, no need to interpolate it.

cc06076 fix this

@viirya Any other need change?

I'm not sure about #35095 (comment).

thank @viirya , ping @dongjoon-hyun @williamhyun

srowen

Looks OK pending tests

LuciferYang · 2022-01-11T15:43:45Z

8d0a078 merge with master

LuciferYang · 2022-01-12T02:28:04Z

Looks OK pending tests

GA passed

srowen · 2022-01-13T00:11:50Z

Merged to master

LuciferYang · 2022-01-13T02:54:35Z

thanks all

…se log4j2 api ### 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 let the log behavior using log4j2.x be the same as that using log4j1.x before. In fact, the `configTestLog4j` method behavior before this pr is invalid because `PropertyConfigurator.configure` method in `org.apache.logging.log4j:log4j-1.2-api` is an empty method as follows: https://github.com/apache/logging-log4j2/blob/491a0b3787975b6fc95b6a8cb3da76dc7517c65f/log4j-1.2-api/src/main/java/org/apache/log4j/PropertyConfigurator.java#L39-L47 Another change of this pr is rename the method name from `configTestLog4j` to `configTestLog4j2`. ### Why are the changes needed? Clean up the `org.apache.log4j` imports left in Spark internal and let `configTestLog4j` method behavior keep consistent between log4j1.x and log4j2.x. ### Does this PR introduce _any_ user-facing change? The `configTestLog4j` method in `TestUtils` rename to `configTestLog4j2` ### How was this patch tested? - Pass the Jenkins or GitHub Action - Manual test Run the test cases using `configTestLog4j` method in the following 3 scenarios: 1. without this pr to test log4j2.x 2. with this pr to test log4j2.x 3. run `git reset --hard 1922798` to test log4j1.x For example `WholeStageCodegenSparkSubmitSuite`, run ``` mvn clean install -DskipTests -pl sql/core -am mvn test -pl sql/core -Dtest=none -DwildcardSuites=org.apache.spark.sql.execution.WholeStageCodegenSparkSubmitSuite ``` Scenario 1 does not print any logs to the console, scenario 2 and scenario 3 will print similar logs to the console Closes apache#35095 from LuciferYang/refactor-configTestLog4j. Authored-by: yangjie01 <[email protected]> Signed-off-by: Sean Owen <[email protected]>

…se log4j2 api 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 let the log behavior using log4j2.x be the same as that using log4j1.x before. In fact, the `configTestLog4j` method behavior before this pr is invalid because `PropertyConfigurator.configure` method in `org.apache.logging.log4j:log4j-1.2-api` is an empty method as follows: https://github.com/apache/logging-log4j2/blob/491a0b3787975b6fc95b6a8cb3da76dc7517c65f/log4j-1.2-api/src/main/java/org/apache/log4j/PropertyConfigurator.java#L39-L47 Another change of this pr is rename the method name from `configTestLog4j` to `configTestLog4j2`. Clean up the `org.apache.log4j` imports left in Spark internal and let `configTestLog4j` method behavior keep consistent between log4j1.x and log4j2.x. The `configTestLog4j` method in `TestUtils` rename to `configTestLog4j2` - Pass the Jenkins or GitHub Action - Manual test Run the test cases using `configTestLog4j` method in the following 3 scenarios: 1. without this pr to test log4j2.x 2. with this pr to test log4j2.x 3. run `git reset --hard 1922798` to test log4j1.x For example `WholeStageCodegenSparkSubmitSuite`, run ``` mvn clean install -DskipTests -pl sql/core -am mvn test -pl sql/core -Dtest=none -DwildcardSuites=org.apache.spark.sql.execution.WholeStageCodegenSparkSubmitSuite ``` Scenario 1 does not print any logs to the console, scenario 2 and scenario 3 will print similar logs to the console Closes apache#35095 from LuciferYang/refactor-configTestLog4j. Authored-by: yangjie01 <[email protected]> Signed-off-by: Sean Owen <[email protected]>

…se log4j2 api ### 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 let the log behavior using log4j2.x be the same as that using log4j1.x before. In fact, the `configTestLog4j` method behavior before this pr is invalid because `PropertyConfigurator.configure` method in `org.apache.logging.log4j:log4j-1.2-api` is an empty method as follows: https://github.com/apache/logging-log4j2/blob/491a0b3787975b6fc95b6a8cb3da76dc7517c65f/log4j-1.2-api/src/main/java/org/apache/log4j/PropertyConfigurator.java#L39-L47 Another change of this pr is rename the method name from `configTestLog4j` to `configTestLog4j2`. ### Why are the changes needed? Clean up the `org.apache.log4j` imports left in Spark internal and let `configTestLog4j` method behavior keep consistent between log4j1.x and log4j2.x. ### Does this PR introduce _any_ user-facing change? The `configTestLog4j` method in `TestUtils` rename to `configTestLog4j2` ### How was this patch tested? - Pass the Jenkins or GitHub Action - Manual test Run the test cases using `configTestLog4j` method in the following 3 scenarios: 1. without this pr to test log4j2.x 2. with this pr to test log4j2.x 3. run `git reset --hard 1922798` to test log4j1.x For example `WholeStageCodegenSparkSubmitSuite`, run ``` mvn clean install -DskipTests -pl sql/core -am mvn test -pl sql/core -Dtest=none -DwildcardSuites=org.apache.spark.sql.execution.WholeStageCodegenSparkSubmitSuite ``` Scenario 1 does not print any logs to the console, scenario 2 and scenario 3 will print similar logs to the console Closes apache#35095 from LuciferYang/refactor-configTestLog4j. Authored-by: yangjie01 <[email protected]> Signed-off-by: Sean Owen <[email protected]>

…se log4j2 api ### 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 let the log behavior using log4j2.x be the same as that using log4j1.x before. In fact, the `configTestLog4j` method behavior before this pr is invalid because `PropertyConfigurator.configure` method in `org.apache.logging.log4j:log4j-1.2-api` is an empty method as follows: https://github.com/apache/logging-log4j2/blob/491a0b3787975b6fc95b6a8cb3da76dc7517c65f/log4j-1.2-api/src/main/java/org/apache/log4j/PropertyConfigurator.java#L39-L47 Another change of this pr is rename the method name from `configTestLog4j` to `configTestLog4j2`. ### Why are the changes needed? Clean up the `org.apache.log4j` imports left in Spark internal and let `configTestLog4j` method behavior keep consistent between log4j1.x and log4j2.x. ### Does this PR introduce _any_ user-facing change? The `configTestLog4j` method in `TestUtils` rename to `configTestLog4j2` ### How was this patch tested? - Pass the Jenkins or GitHub Action - Manual test Run the test cases using `configTestLog4j` method in the following 3 scenarios: 1. without this pr to test log4j2.x 2. with this pr to test log4j2.x 3. run `git reset --hard 1922798` to test log4j1.x For example `WholeStageCodegenSparkSubmitSuite`, run ``` mvn clean install -DskipTests -pl sql/core -am mvn test -pl sql/core -Dtest=none -DwildcardSuites=org.apache.spark.sql.execution.WholeStageCodegenSparkSubmitSuite ``` Scenario 1 does not print any logs to the console, scenario 2 and scenario 3 will print similar logs to the console Closes apache#35095 from LuciferYang/refactor-configTestLog4j. Authored-by: yangjie01 <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit f3eedaf)

LuciferYang added 2 commits January 4, 2022 14:59

refactor first

97b5a48

rename to configTestLog4j2

8572b87

github-actions bot added CORE SQL labels Jan 4, 2022

viirya reviewed Jan 4, 2022

View reviewed changes

viirya reviewed Jan 5, 2022

View reviewed changes

LuciferYang added 3 commits January 5, 2022 14:52

fix viirya's commnets

cc06076

Merge branch 'upmaster' into refactor-configTestLog4j

8deac76

Merge branch 'upmaster' into refactor-configTestLog4j

93ee89a

srowen requested changes Jan 11, 2022

View reviewed changes

Merge branch 'upmaster' into refactor-configTestLog4j

8d0a078

LuciferYang requested a review from srowen January 12, 2022 02:28

srowen approved these changes Jan 12, 2022

View reviewed changes

srowen closed this in f3eedaf Jan 13, 2022

LuciferYang deleted the refactor-configTestLog4j branch October 22, 2023 07:43

[SPARK-37805][TESTS] Refactor TestUtils#configTestLog4j method to use log4j2 api #35095

[SPARK-37805][TESTS] Refactor TestUtils#configTestLog4j method to use log4j2 api #35095

Uh oh!

Conversation

LuciferYang commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

LuciferYang commented Jan 4, 2022

Uh oh!

dongjoon-hyun commented Jan 4, 2022

Uh oh!

dongjoon-hyun commented Jan 4, 2022

Uh oh!

LuciferYang commented Jan 4, 2022

Uh oh!

LuciferYang commented Jan 4, 2022

Uh oh!

LuciferYang commented Jan 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Jan 11, 2022

Uh oh!

LuciferYang commented Jan 12, 2022

Uh oh!

srowen commented Jan 13, 2022

Uh oh!

LuciferYang commented Jan 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api #35095

[SPARK-37805][TESTS] Refactor `TestUtils#configTestLog4j` method to use log4j2 api #35095

LuciferYang commented Jan 4, 2022 •

edited

Loading

LuciferYang Jan 5, 2022 •

edited

Loading

LuciferYang Jan 5, 2022 •

edited

Loading