[SPARK-6305][BUILD][FOLLOWUP] Some cleanups for log4j properties #34959

viirya · 2021-12-20T09:51:18Z

What changes were proposed in this pull request?

This is a followup of SPARK-6305. Some places in build files (maven and sbt) which remove log4j properties files should be updated. Some tests programmingly write log4j properties should be updated to log4j 2 syntax too.

Why are the changes needed?

To clean up build files and migrate some tests to log4j2 syntax.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass all tests.

SparkQA · 2021-12-20T10:41:09Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50871/

SparkQA · 2021-12-20T11:35:55Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50871/

dongjoon-hyun

+1, LGTM (if CI passed).

viirya · 2021-12-20T17:03:05Z

Thanks @dongjoon-hyun . Wait for CI.

SparkQA · 2021-12-20T17:42:14Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50877/

SparkQA · 2021-12-20T18:39:08Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50877/

attilapiros · 2021-12-20T19:16:11Z

I think there is a related error in the kubernetes integration tests:

  <testcase
  name="Verify logging configuration is picked from the provided SPARK_CONF_DIR/log4j.properties" classname="org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite" time="204.286">
    <failure
    message="The code passed to eventually never returned normally. Attempted 180 times over 3.000611025833334 minutes. Last failure message: &quot;++ id -u&amp;#010;+ myuid=185&amp;#010;++ id -g&amp;#010;+ mygid=0&amp;#01
0;+ set +e&amp;#010;++ getent passwd 185&amp;#010;+ uidentry=&amp;#010;+ set -e&amp;#010;+ '[' -z '' ']'&amp;#010;+ ...

It is around here:

spark/resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/SparkConfPropagateSuite.scala

Lines 30 to 45 in 14a9d84

    
           val loggingConfigFileName = "log-config-test-log4j.properties" 
        
           val loggingConfURL: URL = this.getClass.getClassLoader.getResource(loggingConfigFileName) 
        
           assert(loggingConfURL != null, "Logging configuration file not available.") 
        
           val content = Source.createBufferedSource(loggingConfURL.openStream()).getLines().mkString("\n") 
        
           val logConfFilePath = s"${sparkHomeDir.toFile}/conf/log4j.properties" 
        
           try { 
        
             Files.write(new File(logConfFilePath).toPath, content.getBytes) 
        
             sparkAppConf.set("spark.driver.extraJavaOptions", "-Dlog4j.debug") 
        
             sparkAppConf.set("spark.executor.extraJavaOptions", "-Dlog4j.debug") 
        
             sparkAppConf.set("spark.kubernetes.executor.deleteOnTermination", "false") 
        
             val log4jExpectedLog = 
        
               s"log4j: Reading configuration from URL file:/opt/spark/conf/log4j.properties"

Switching on DEBUG for log4j initialization does not trigger the logging of the expected output.

SparkQA · 2021-12-20T20:33:58Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50881/

SparkQA · 2021-12-20T20:34:29Z

Test build #146402 has finished for PR 34959 at commit cec77a4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2021-12-20T21:10:33Z

Hmm, I changed the log4j properties file for the k8s integration test. But seems the test still cannot get the log4j 2 configuration file. Not sure why now. I will merge this first and keep looking at the k8s integration test issue.

SparkQA · 2021-12-20T21:31:09Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50881/

SparkQA · 2021-12-20T22:33:13Z

Test build #146406 has finished for PR 34959 at commit 6ae05e1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2021-12-20T22:38:13Z

Jenkins tests were passed. Thanks for reviewing. Merging to master.

LuciferYang · 2021-12-21T08:08:05Z

@viirya It seems that test failure still exists in hive-thriftserver module:

https://amplab.cs.berkeley.edu/jenkins/job/spark-master-test-maven-hadoop-3.2/3527/consoleFull

viirya · 2021-12-21T08:17:13Z

Which one you mean? I don't see failed test in test results.

LuciferYang · 2021-12-21T08:26:01Z

=========================================
End HiveThriftServer2Suite failure output
=========================================
       
22:53:44.821 INFO org.apache.spark.sql.hive.thriftserver.HiveThriftBinaryServerSuite: HiveThriftServer2 stopped
22:53:44.827 WARN org.apache.spark.sql.hive.thriftserver.HiveThriftBinaryServerSuite: 

===== POSSIBLE THREAD LEAK IN SUITE o.a.s.sql.hive.thriftserver.HiveThriftBinaryServerSuite, threads: Thread-13 (daemon=true), Thread-12 (daemon=true) =====

*** RUN ABORTED ***
  java.util.concurrent.TimeoutException: Futures timed out after [3 minutes]
  at scala.concurrent.impl.Promise$DefaultPromise.ready(Promise.scala:259)
  at scala.concurrent.impl.Promise$DefaultPromise.result(Promise.scala:263)
  at org.apache.spark.util.ThreadUtils$.awaitResult(ThreadUtils.scala:293)
  at org.apache.spark.sql.hive.thriftserver.HiveThriftServer2TestBase.startThriftServer(HiveThriftServer2Suites.scala:1344)
  at org.apache.spark.sql.hive.thriftserver.HiveThriftServer2TestBase.$anonfun$beforeAll$4(HiveThriftServer2Suites.scala:1397)
  at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)
  at scala.util.Try$.apply(Try.scala:213)
  at org.apache.spark.sql.hive.thriftserver.HiveThriftServer2TestBase.$anonfun$beforeAll$3(HiveThriftServer2Suites.scala:1396)
  at scala.util.Failure.orElse(Try.scala:224)
  at org.apache.spark.sql.hive.thriftserver.HiveThriftServer2TestBase.$anonfun$beforeAll$2(HiveThriftServer2Suites.scala:1394)

viirya · 2021-12-21T08:29:27Z

I will try to reproduce it locally.

LuciferYang · 2021-12-21T08:29:54Z

And there are some error log as follows:

Discovery starting.
2021-12-20 22:41:21,477 ScalaTest-main ERROR Filters contains invalid attributes "onMatch", "onMismatch"
2021-12-20 22:41:21,492 ScalaTest-main ERROR Filters contains invalid attributes "onMatch", "onMismatch"
Discovery completed in 1 second, 557 milliseconds.
Run starting. Expected test count is: 551

It seems that there is some thing wrong with log4j2 configuration.

viirya · 2021-12-21T08:31:18Z

The log4j error I have seen locally, but it doesn't affect SBT test. Not sure if it is related.

LuciferYang · 2021-12-21T08:31:39Z

The log4j error I have seen locally, but it doesn't affect SBT test. Not sure if it is related.

ok

viirya · 2021-12-21T08:33:37Z

Do you see other issues? Seems other tests are all passed on maven, right?

LuciferYang · 2021-12-21T08:40:16Z

Do you see other issues? Seems other tests are all passed on maven, right?

It seems so, only hive-thriftserver module test fail now

LuciferYang · 2021-12-21T09:00:39Z

...ftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2Suites.scala

+          |appender.console.layout.type = PatternLayout
+          |appender.console.layout.pattern = %d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n
        """.stripMargin,
        new File(s"$tempLog4jConf/log4j.properties"),


change new File(s"$tempLog4jConf/log4j.properties") to new File(s"$tempLog4jConf/log4j2.properties"), then HiveThriftBinaryServerSuite no longer hang

However, it seems that all logs are printed to the console, which is different from the previous behavior

SBT cannot reproduce it. But we should use log4j2.properties.

I corrected this together in #34965.

This is a followup of SPARK-6305. Some places in build files (maven and sbt) which remove log4j properties files should be updated. Some tests programmingly write log4j properties should be updated to log4j 2 syntax too. To clean up build files and migrate some tests to log4j2 syntax. No Pass all tests. Closes apache#34959 from viirya/log4j2_followup. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]>

### What changes were proposed in this pull request? This is a followup of SPARK-6305. Some places in build files (maven and sbt) which remove log4j properties files should be updated. Some tests programmingly write log4j properties should be updated to log4j 2 syntax too. ### Why are the changes needed? To clean up build files and migrate some tests to log4j2 syntax. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass all tests. Closes apache#34959 from viirya/log4j2_followup. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]>

### What changes were proposed in this pull request? This is a followup of SPARK-6305. Some places in build files (maven and sbt) which remove log4j properties files should be updated. Some tests programmingly write log4j properties should be updated to log4j 2 syntax too. ### Why are the changes needed? To clean up build files and migrate some tests to log4j2 syntax. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass all tests. Closes apache#34959 from viirya/log4j2_followup. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]> (cherry picked from commit 9f6062a)

Some cleanup

14a9d84

github-actions bot added BUILD DOCS INFRA KUBERNETES SQL YARN labels Dec 20, 2021

This comment has been minimized.

Sign in to view

Fix log4j system property

cec77a4

dongjoon-hyun approved these changes Dec 20, 2021

View reviewed changes

viirya added 2 commits December 20, 2021 11:29

Update log4j2 system properties.

5e74d61

Update kubernetes integration test log4j properties.

6ae05e1

viirya closed this in 9f6062a Dec 20, 2021

LuciferYang reviewed Dec 21, 2021

View reviewed changes

[SPARK-6305][BUILD][FOLLOWUP] Some cleanups for log4j properties #34959

[SPARK-6305][BUILD][FOLLOWUP] Some cleanups for log4j properties #34959

Uh oh!

Conversation

viirya commented Dec 20, 2021

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!

SparkQA commented Dec 20, 2021

Uh oh!

SparkQA commented Dec 20, 2021

Uh oh!

This comment has been minimized.

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented Dec 20, 2021

Uh oh!

SparkQA commented Dec 20, 2021

Uh oh!

SparkQA commented Dec 20, 2021

Uh oh!

attilapiros commented Dec 20, 2021

Uh oh!

SparkQA commented Dec 20, 2021

Uh oh!

SparkQA commented Dec 20, 2021

Uh oh!

viirya commented Dec 20, 2021

Uh oh!

SparkQA commented Dec 20, 2021

Uh oh!

SparkQA commented Dec 20, 2021

Uh oh!

viirya commented Dec 20, 2021

Uh oh!

LuciferYang commented Dec 21, 2021

Uh oh!

viirya commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LuciferYang commented Dec 21, 2021

Uh oh!

viirya commented Dec 21, 2021

Uh oh!

LuciferYang commented Dec 21, 2021

Uh oh!

viirya commented Dec 21, 2021

Uh oh!

LuciferYang commented Dec 21, 2021

Uh oh!

viirya commented Dec 21, 2021

Uh oh!

LuciferYang commented Dec 21, 2021

Uh oh!

LuciferYang Dec 21, 2021 • 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 Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

viirya Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

viirya Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viirya commented Dec 21, 2021 •

edited

Loading

LuciferYang Dec 21, 2021 •

edited

Loading