[SPARK-26443][CORE] Use ConfigEntry for hardcoded configs for history category. #23384

ueshin · 2018-12-26T06:59:45Z

What changes were proposed in this pull request?

This pr makes hardcoded "spark.history" configs to use ConfigEntry and put them in History config object.

How was this patch tested?

Existing tests.

SparkQA · 2018-12-26T08:05:02Z

Test build #100448 has finished for PR 23384 at commit 613dad0.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2018-12-26T08:05:49Z

Jenkins, retest this please.

HyukjinKwon

Looks fine. Adding @vanzin

SparkQA · 2018-12-26T12:45:54Z

Test build #100450 has finished for PR 23384 at commit 613dad0.

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

dongjoon-hyun

+1, LGTM.

mgaido91 · 2018-12-28T11:56:34Z

core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala

    val conf = new SparkConf()
-      .set("spark.history.fs.logDirectory", logDir)
-      .set("spark.history.fs.update.interval", "0")
+      .set(EVENT_LOG_DIR, logDir)


EVENT_LOG_DIR.key?

I think we can use the config entry as a key directly except for configs the string expression of which is more readable like timeConf or bytesConf. WDYT?

actually I am fine with both, this was rather a nit, I just meant to have one notation instead of both them if possible, in order to avoid confusion. But if you prefer this way, I am fine with it, thanks.

mgaido91 · 2018-12-28T11:56:43Z

core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala

    stop()
    val myConf = new SparkConf()
-      .set("spark.history.fs.logDirectory", logDir.getAbsolutePath)
+      .set(EVENT_LOG_DIR, logDir.getAbsolutePath)


mgaido91 · 2018-12-28T11:57:21Z

core/src/test/scala/org/apache/spark/deploy/history/HistoryServerArgumentsSuite.scala

  private val conf = new SparkConf()
-    .set("spark.history.fs.logDirectory", logDir.getAbsolutePath)
-    .set("spark.history.fs.updateInterval", "1")
+    .set(EVENT_LOG_DIR, logDir.getAbsolutePath)


nit: .key?

mgaido91 · 2018-12-28T11:57:27Z

core/src/test/scala/org/apache/spark/deploy/history/HistoryServerArgumentsSuite.scala

-    .set("spark.history.fs.logDirectory", logDir.getAbsolutePath)
-    .set("spark.history.fs.updateInterval", "1")
+    .set(EVENT_LOG_DIR, logDir.getAbsolutePath)
+    .set(UPDATE_INTERVAL_S, 1L)


mgaido91 · 2018-12-28T11:58:02Z

core/src/test/scala/org/apache/spark/deploy/history/FsHistoryProviderSuite.scala

    val testConf = new SparkConf()
-    testConf.set("spark.history.fs.logDirectory",
-      Utils.createTempDir(namePrefix = "eventLog").getAbsolutePath())
+    testConf.set(EVENT_LOG_DIR, Utils.createTempDir(namePrefix = "eventLog").getAbsolutePath())


mgaido91 · 2018-12-28T11:59:04Z

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

      |
      |  spark.history.fs.logDirectory      Directory where app logs are stored
      |                                     (default: file:/tmp/spark-events)
-      |  spark.history.fs.updateInterval    How often to reload log data from storage


do we need to change this? Seems this pattern is quite used too...

Seems like spark.history.fs.updateInterval is deprecated, but let me revert it for now.

Ur, it is good to remove the deprecated one as many as possible in Spark 3.0. When I searched this string during reviews, there were only two instances; here and deprecation checking test case. @mgaido91 , what do you mean by quite used? We already accepted this deprecated one and new one.

I see only now that it was deprecated, sorry. Then, this is ok, sorry for the comment.

@dongjoon-hyun I meant that camel case notation for the last element is quite widespread also for other configs, so I didn't see the need for changing the notation.

Then should I update to include this change back?

I think it's okie. It's deprecated and we should get rid of it in the doc basically. I see no issue. Yea, I think it can be included back. LGTM without that change as well.

mgaido91 · 2018-12-28T12:01:32Z

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

  val DEFAULT_LOG_DIR = "file:/tmp/spark-events"

  val EVENT_LOG_DIR = ConfigBuilder("spark.history.fs.logDirectory")
    .stringConf


not related to this PR, but EVENT_LOG_DIR doesn't seem a great name to me for this property, as we have a spark.eventLog.dir. Shall we update it to HISTORY_LOG_DIR or something similar in order to disambiguate?

I'm fine with the name HISTORY_LOG_DIR. Let me update it.
cc @HyukjinKwon @srowen @dongjoon-hyun @vanzin

Yep. HISTORY_LOG_DIR sounds good to me, too.

SparkQA · 2018-12-28T17:28:49Z

Test build #100502 has finished for PR 23384 at commit a9208ef.

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

HyukjinKwon · 2018-12-29T10:06:39Z

LGTM

This reverts commit a364017.

SparkQA · 2018-12-30T01:24:37Z

Test build #100541 has finished for PR 23384 at commit 100902f.

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

dongjoon-hyun · 2018-12-30T01:32:21Z

Thank you, @ueshin , @HyukjinKwon , @srowen , @mgaido91 .
Merged to master.

… category. ## What changes were proposed in this pull request? This pr makes hardcoded "spark.history" configs to use `ConfigEntry` and put them in `History` config object. ## How was this patch tested? Existing tests. Closes apache#23384 from ueshin/issues/SPARK-26443/hardcoded_history_configs. Authored-by: Takuya UESHIN <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

Use ConfigEntry for hardcoded configs for history category.

613dad0

HyukjinKwon approved these changes Dec 26, 2018

View reviewed changes

srowen approved these changes Dec 26, 2018

View reviewed changes

dongjoon-hyun approved these changes Dec 28, 2018

View reviewed changes

mgaido91 reviewed Dec 28, 2018

View reviewed changes

ueshin added 2 commits December 28, 2018 21:40

Revert a change.

a364017

Rename.

a9208ef

Revert "Revert a change."

100902f

This reverts commit a364017.

dongjoon-hyun approved these changes Dec 29, 2018

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-26443 ][CORE] Use ConfigEntry for hardcoded configs for history category.~~ [SPARK-26443][CORE] Use ConfigEntry for hardcoded configs for history category. Dec 30, 2018

asfgit closed this in e6d3e7d Dec 30, 2018

[SPARK-26443][CORE] Use ConfigEntry for hardcoded configs for history category. #23384

[SPARK-26443][CORE] Use ConfigEntry for hardcoded configs for history category. #23384

Uh oh!

Conversation

ueshin commented Dec 26, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Dec 26, 2018

Uh oh!

ueshin commented Dec 26, 2018

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 26, 2018

Uh oh!

dongjoon-hyun left a comment

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!

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!

HyukjinKwon Dec 29, 2018 • 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!

SparkQA commented Dec 28, 2018

Uh oh!

HyukjinKwon commented Dec 29, 2018

Uh oh!

SparkQA commented Dec 30, 2018

Uh oh!

dongjoon-hyun commented Dec 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

HyukjinKwon Dec 29, 2018 •

edited

Loading