[SPARK-34346][CORE][TESTS][FOLLOWUP] Fix UT by removing core-site.xml by dongjoon-hyun · Pull Request #31515 · apache/spark

dongjoon-hyun · 2021-02-08T00:24:32Z

What changes were proposed in this pull request?

This is a follow-up for SPARK-34346 which causes a flakiness due to core-site.xml test resource file addition. This PR aims to remove the test resource core/src/test/resources/core-site.xml from core module.

Why are the changes needed?

Due to the test resource core-site.xml, YARN UT becomes flaky in GitHub Action and Jenkins.

$ build/sbt "yarn/testOnly *.YarnClusterSuite -- -z SPARK-16414" -Pyarn
...
[info] YarnClusterSuite:
[info] - yarn-cluster should respect conf overrides in SparkHadoopUtil (SPARK-16414, SPARK-23630) *** FAILED *** (20 seconds, 209 milliseconds)
[info]   FAILED did not equal FINISHED (stdout/stderr was not captured) (BaseYarnClusterSuite.scala:210)

To isolate more, we may use SPARK_TEST_HADOOP_CONF_DIR like yarn module's yarn/Client, but it seems an overkill in core module.

// SPARK-23630: during testing, Spark scripts filter out hadoop conf dirs so that user's
// environments do not interfere with tests. This allows a special env variable during
// tests so that custom conf dirs can be used by unit tests.
val confDirs = Seq("HADOOP_CONF_DIR", "YARN_CONF_DIR") ++
  (if (Utils.isTesting) Seq("SPARK_TEST_HADOOP_CONF_DIR") else Nil)

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the CIs.

dongjoon-hyun · 2021-02-08T00:30:11Z

core/src/test/scala/org/apache/spark/SparkContextSuite.scala

    val testKey = "hadoop.tmp.dir"
    val bufferKey = "io.file.buffer.size"
    val hadoopConf0 = new Configuration()
+    hadoopConf0.set(testKey, "/tmp/hive_zero")


This is still a Hadoop conf although it's Overlay property.

dongjoon-hyun · 2021-02-08T00:33:14Z

core/src/test/scala/org/apache/spark/SparkContextSuite.scala

    assert(hiveConfFile != null)
    hadoopConf0.addResource(hiveConfFile)
-    assert(hadoopConf0.get(testKey) === "/tmp/hive_one")
+    assert(hadoopConf0.get(testKey) === "/tmp/hive_zero")


This is okay because Apache Spark UT aims to test line 1170 ~ 1172.

assert(sc.hadoopConfiguration.get(testKey) === "/tmp/hive_one", "hive configs have higher priority than hadoop ones ") assert(sc.hadoopConfiguration.get(bufferKey).toInt === 65536, "spark configs have higher priority than hive ones")

w/ this change, "/tmp/hive_one" in hive-site.xml overlays "/tmp/hive_{user}" not "/tmp/hive_zero", which happens in SparkHadoopUtil.appendS3AndSparkHadoopHiveConfigurations.

As hadoopConf0 is not passed to SparkHadoopUtil, while core-site.xml is, so here to reduce test flakiness, we can remove those asserts for hadoopConf0, as they are just used to check hive-site.xml overrides core-site.xml.

@yaooqinn, can you open a PR for that, or push some changes into this PR? I think the test is not flaky but broken. It doesn't look obvious because we don't always run YarnClusterSuite.

dongjoon-hyun · 2021-02-08T00:45:48Z

Could you review this, @yaooqinn , @HyukjinKwon , @cloud-fan , @srowen ?

srowen

Looks fine. We just added the file right? so it's not going to hurt anything else to remove it.

SparkQA · 2021-02-08T02:08:27Z

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

#31515 (comment)

HyukjinKwon · 2021-02-08T02:31:33Z

Hey guys, let me just merge this for now. I do think the test did not pass and it is broken.

@yaooqinn, can you make another followup to remove the asserts?

HyukjinKwon · 2021-02-08T02:32:44Z

Merged to master, branch-3.1 and branch-3.0.

### What changes were proposed in this pull request? This is a follow-up for SPARK-34346 which causes a flakiness due to `core-site.xml` test resource file addition. This PR aims to remove the test resource `core/src/test/resources/core-site.xml` from `core` module. ### Why are the changes needed? Due to the test resource `core-site.xml`, YARN UT becomes flaky in GitHub Action and Jenkins. ``` $ build/sbt "yarn/testOnly *.YarnClusterSuite -- -z SPARK-16414" -Pyarn ... [info] YarnClusterSuite: [info] - yarn-cluster should respect conf overrides in SparkHadoopUtil (SPARK-16414, SPARK-23630) *** FAILED *** (20 seconds, 209 milliseconds) [info] FAILED did not equal FINISHED (stdout/stderr was not captured) (BaseYarnClusterSuite.scala:210) ``` To isolate more, we may use `SPARK_TEST_HADOOP_CONF_DIR` like `yarn` module's `yarn/Client`, but it seems an overkill in `core` module. ``` // SPARK-23630: during testing, Spark scripts filter out hadoop conf dirs so that user's // environments do not interfere with tests. This allows a special env variable during // tests so that custom conf dirs can be used by unit tests. val confDirs = Seq("HADOOP_CONF_DIR", "YARN_CONF_DIR") ++ (if (Utils.isTesting) Seq("SPARK_TEST_HADOOP_CONF_DIR") else Nil) ``` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. Closes #31515 from dongjoon-hyun/SPARK-34346-2. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org> (cherry picked from commit dcaf62a) Signed-off-by: HyukjinKwon <gurwls223@apache.org>

SparkQA · 2021-02-08T02:41:33Z

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

yaooqinn · 2021-02-08T03:04:28Z

OK~

SparkQA · 2021-02-08T03:48:23Z

Test build #134993 has finished for PR 31515 at commit 189cddf.

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

dongjoon-hyun · 2021-02-08T05:52:51Z

Thank you, @srowen , @yaooqinn , @HyukjinKwon .

[SPARK-34346][CORE][TESTS][FOLLOWUP] Fix UT by removing core-site.xml

189cddf

github-actions bot added the CORE label Feb 8, 2021

dongjoon-hyun commented Feb 8, 2021

View reviewed changes

dongjoon-hyun mentioned this pull request Feb 8, 2021

[SPARK-34346][CORE][SQL] io.file.buffer.size set by spark.buffer.size will override by loading hive-site.xml accidentally may cause perf regression #31460

Closed

srowen reviewed Feb 8, 2021

View reviewed changes

HyukjinKwon previously approved these changes Feb 8, 2021

View reviewed changes

HyukjinKwon closed this in dcaf62a Feb 8, 2021

dongjoon-hyun deleted the SPARK-34346-2 branch February 8, 2021 05:52

Conversation

dongjoon-hyun commented Feb 8, 2021 • 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!

dongjoon-hyun Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

yaooqinn Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Feb 8, 2021

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 8, 2021

Uh oh!

HyukjinKwon commented Feb 8, 2021

Uh oh!

HyukjinKwon commented Feb 8, 2021

Uh oh!

SparkQA commented Feb 8, 2021

Uh oh!

yaooqinn commented Feb 8, 2021

Uh oh!

SparkQA commented Feb 8, 2021

Uh oh!

dongjoon-hyun commented Feb 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dongjoon-hyun commented Feb 8, 2021 •

edited

Loading