[SPARK-26491][CORE][TEST] Use ConfigEntry for hardcoded configs for test categories #23413

mgaido91 · 2018-12-30T14:26:31Z

What changes were proposed in this pull request?

The PR makes hardcoded spark.test and spark.testing configs to use ConfigEntry and put them in the config package.

How was this patch tested?

existing UTs

…est categories

SparkQA · 2018-12-30T18:07:25Z

Test build #100559 has finished for PR 23413 at commit 73618ce.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-12-30T23:28:47Z

Test build #100570 has finished for PR 23413 at commit ba41657.

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

mgaido91 · 2019-01-01T17:10:12Z

cc @dongjoon-hyun @ueshin

vanzin · 2019-01-02T21:28:27Z

core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala

@@ -267,7 +268,7 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
    }

    // Disable the background thread during tests.
-    if (!conf.contains("spark.testing")) {
+    if (!conf.contains(IS_TESTING)) {


Can probably be Utils.isTesting.

I don't think so, here we are checking the SparkConf, while in Utils.isTesting we check the system properties. So I don't think it is doable.

Well, that's kinda the point. spark.testing is set as a system property by the build scripts. SparkConf just inherits system properties, which is why you can also check it there.

(I saw your comment about the SHS tests removing the testing conf, let me take a look...)

vanzin · 2019-01-02T21:28:42Z

core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala

@@ -103,7 +104,7 @@ private[deploy] class Worker(
  private val CLEANUP_NON_SHUFFLE_FILES_ENABLED =
    conf.getBoolean("spark.storage.cleanupFilesAfterExecutorExit", true)

-  private val testing: Boolean = sys.props.contains("spark.testing")
+  private val testing: Boolean = sys.props.contains(IS_TESTING.key)


vanzin · 2019-01-02T21:28:56Z

core/src/main/scala/org/apache/spark/executor/ProcfsMetricsGetter.scala

@@ -43,7 +44,7 @@ private[spark] case class ProcfsMetrics(
 // project.
 private[spark] class ProcfsMetricsGetter(procfsDir: String = "/proc/") extends Logging {
  private val procfsStatFile = "stat"
-  private val testing = sys.env.contains("SPARK_TESTING") || sys.props.contains("spark.testing")
+  private val testing = sys.env.contains("SPARK_TESTING") || sys.props.contains(IS_TESTING.key)


vanzin · 2019-01-02T21:29:13Z

core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala

@@ -202,7 +203,7 @@ class TaskMetrics private[spark] () extends Serializable {
  }

  // Only used for test
-  private[spark] val testAccum = sys.props.get("spark.testing").map(_ => new LongAccumulator)
+  private[spark] val testAccum = sys.props.get(IS_TESTING.key).map(_ => new LongAccumulator)


I agree it is doable but the code would become an if which seems to me less clean than the current code

vanzin · 2019-01-02T21:32:24Z

core/src/main/scala/org/apache/spark/internal/config/Tests.scala

+      .longConf
+      .createWithDefault(100)
+
+  val IS_TESTING = ConfigBuilder("spark.testing")


Seems to me that if you use Utils.isTesting everywhere, you don't need this. I took a look at the places where IS_TESTING is set explicitly, and they should work with that code removed. Unless there's a test that relies on isTesting being false, that should be doable.

spark.testing is set by the build scripts, so tests shouldn't need to set it to true.

there are cases when it is removed (see the HistoryServer one) and when it is set to false (in the Kubernetes one). So I don't think it is doable.

I actually tried out the k8s tests without that line and they work fine, which tells me it's not needed. Same for a lot of other explicit checks and setting of that property. I think eventually we should fix all this code to not mess with that property, especially since it's so inconsistent (SparkConf vs. system properties vs. env variable).

The SHS test does need to remove the conf, though; it could be implemented differently, but at that point it's probably better to do a separate change and leave this one as a simple "make everybody use constants for this" change.

+1 for this. I think we should check case by case if some of them are not needed and removed, but I agree we best do that in dedicated PRs for each of these cases.

SparkQA · 2019-01-03T15:17:55Z

Test build #100684 has finished for PR 23413 at commit b54a07b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-01-03T19:55:24Z

Test build #100694 has finished for PR 23413 at commit a0ae515.

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

vanzin

looks good with a minor comment.

vanzin · 2019-01-03T21:51:39Z

core/src/main/scala/org/apache/spark/util/SizeEstimator.scala

@@ -126,8 +127,8 @@ object SizeEstimator extends Logging {
  private def getIsCompressedOops: Boolean = {
    // This is only used by tests to override the detection of compressed oops. The test


Given this comment I wonder if changing this one config is needed. (I also don't see any SparkConf available in this context, so looks that even that route is not possible.)

there are cases when this config is set in the tests. It seems it is useless indeed, since we read it only here as a system property. I think we can consider removing the useless set as part of another PR related to that.

Otherwise we can try and replace the config with a string TEST_USE_COMPRESSED_OOPS_KEY which we use when setting/reading the system property. WDYT?

I didn't mean it's useless (it might be, didn't look), just that it could be kept as a system property instead of a config constant (since it's never set in or read from SparkConf). Using a string constant sounds fine, but given what I think is the goal of these changes (see SPARK-26060), that wouldn't really change much.

the point is: it is set in SparkConf in some tests. But this is probably useless because we read it only here as a system property.

I see; then it can probably be just removed from those tests.

SparkQA · 2019-01-05T17:31:52Z

Test build #100797 has finished for PR 23413 at commit 10e0237.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-01-06T01:36:39Z

Test build #100806 has finished for PR 23413 at commit 0df2dfe.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2019-01-06T15:20:21Z

retest this please

SparkQA · 2019-01-06T19:11:57Z

Test build #100834 has finished for PR 23413 at commit 0df2dfe.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2019-01-06T19:13:48Z

retest this please

SparkQA · 2019-01-06T23:44:23Z

Test build #100843 has finished for PR 23413 at commit 0df2dfe.

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

vanzin · 2019-01-07T23:35:11Z

Merging to master.

dongjoon-hyun · 2019-01-10T04:59:02Z

...ts/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesTestComponents.scala

@@ -67,7 +68,7 @@ private[spark] class KubernetesTestComponents(defaultClient: DefaultKubernetesCl
      .set("spark.executors.instances", "1")
      .set("spark.app.name", "spark-test-app")
      .set("spark.ui.enabled", "true")
-      .set("spark.testing", "false")
+      .set(IS_TESTING, false)


Hi, All.
This causes a compilation failure because this is SparkAppConf. I made a PR, #23505 .

…est categories ## What changes were proposed in this pull request? The PR makes hardcoded `spark.test` and `spark.testing` configs to use `ConfigEntry` and put them in the config package. ## How was this patch tested? existing UTs Closes apache#23413 from mgaido91/SPARK-26491. Authored-by: Marco Gaido <marcogaido91@gmail.com> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

[SPARK-26491][CORE][TEST] Use ConfigEntry for hardcoded configs for t…

73618ce

…est categories

fix

ba41657

vanzin reviewed Jan 2, 2019

View reviewed changes

address comments

b54a07b

fix failure

a0ae515

vanzin reviewed Jan 3, 2019

View reviewed changes

mgaido91 added 2 commits January 5, 2019 18:19

Merge branch 'master' of github.com:apache/spark into SPARK-26491

10d1a97

address comment

10e0237

fix build

0df2dfe

asfgit closed this in 1a64152 Jan 7, 2019

dongjoon-hyun reviewed Jan 10, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-26491][CORE][TEST] Use ConfigEntry for hardcoded configs for test categories #23413

[SPARK-26491][CORE][TEST] Use ConfigEntry for hardcoded configs for test categories #23413

mgaido91 commented Dec 30, 2018

SparkQA commented Dec 30, 2018

SparkQA commented Dec 30, 2018

mgaido91 commented Jan 1, 2019

vanzin Jan 2, 2019

mgaido91 Jan 3, 2019

vanzin Jan 3, 2019

vanzin Jan 3, 2019

vanzin Jan 2, 2019

vanzin Jan 2, 2019

vanzin Jan 2, 2019

mgaido91 Jan 3, 2019

vanzin Jan 2, 2019

mgaido91 Jan 3, 2019

vanzin Jan 3, 2019

mgaido91 Jan 4, 2019

SparkQA commented Jan 3, 2019

SparkQA commented Jan 3, 2019

vanzin left a comment

vanzin Jan 3, 2019

mgaido91 Jan 4, 2019

vanzin Jan 4, 2019

mgaido91 Jan 4, 2019

vanzin Jan 4, 2019

SparkQA commented Jan 5, 2019

SparkQA commented Jan 6, 2019

mgaido91 commented Jan 6, 2019

SparkQA commented Jan 6, 2019

mgaido91 commented Jan 6, 2019

SparkQA commented Jan 6, 2019

vanzin commented Jan 7, 2019

dongjoon-hyun Jan 10, 2019

		@@ -126,8 +127,8 @@ object SizeEstimator extends Logging {
		private def getIsCompressedOops: Boolean = {
		// This is only used by tests to override the detection of compressed oops. The test

[SPARK-26491][CORE][TEST] Use ConfigEntry for hardcoded configs for test categories #23413

[SPARK-26491][CORE][TEST] Use ConfigEntry for hardcoded configs for test categories #23413

Conversation

mgaido91 commented Dec 30, 2018

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Dec 30, 2018

SparkQA commented Dec 30, 2018

mgaido91 commented Jan 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 3, 2019

SparkQA commented Jan 3, 2019

vanzin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 5, 2019

SparkQA commented Jan 6, 2019

mgaido91 commented Jan 6, 2019

SparkQA commented Jan 6, 2019

mgaido91 commented Jan 6, 2019

SparkQA commented Jan 6, 2019

vanzin commented Jan 7, 2019

Choose a reason for hiding this comment