[SPARK-23820][CORE] Enable use of long form of callsite in logs by michaelmior · Pull Request #21433 · apache/spark

michaelmior · 2018-05-25T15:53:57Z

This adds an option to event logging to include the long form of the callsite instead of the short form.

kiszk · 2018-05-25T16:07:34Z

Could you please add the description for this PR?

michaelmior · 2018-05-25T16:13:37Z

Done!

srowen · 2018-07-02T14:00:26Z

Although maybe not widely used, I could see allowing control of this via an undocumented param

SparkQA · 2018-07-02T17:53:27Z

Test build #4200 has finished for PR 21433 at commit 9800d2e.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

michaelmior · 2018-07-02T19:25:07Z

Rebased on top of master. The failing test is unrelated.

SparkQA · 2018-07-03T17:10:10Z

Test build #4203 has finished for PR 21433 at commit 245181a.

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

michaelmior · 2018-07-03T17:16:45Z

@srowen Yes, I don't expect it will be widely used but I've personally found it helpful in some performance debugging and it's a fairly low impact change. I was just hoping to avoid having to keep applying this patch and doing my own build of Spark in the future :)

SparkQA · 2018-07-04T01:01:48Z

Test build #4205 has finished for PR 21433 at commit 245181a.

This patch fails PySpark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2018-07-04T05:56:22Z

Test build #4206 has finished for PR 21433 at commit 245181a.

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

srowen · 2018-07-05T13:32:33Z

Merged to master

michaelmior · 2018-07-05T13:54:29Z

Thanks @srowen!

gatorsmile · 2018-09-10T06:26:08Z

core/src/main/scala/org/apache/spark/storage/RDDInfo.scala

    val parentIds = rdd.dependencies.map(_.rdd.id)
+    val callSite = callsiteForm match {
+      case "short" => rdd.creationSite.shortForm
+      case "long" => rdd.creationSite.longForm


If the users input is neither short nor long, we will get an exception, right?

cc @jiangxb1987 @cloud-fan

yea, usually we will define an enum and verify the given config value is valid or not.

For this particular case, I think we can just define a boolean flag: spark.eventLog.callsite.longForm.enabled.

gatorsmile · 2018-09-10T06:28:53Z

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

    ConfigBuilder("spark.eventLog.overwrite").booleanConf.createWithDefault(false)

+  private[spark] val EVENT_LOG_CALLSITE_FORM =
+    ConfigBuilder("spark.eventLog.callsite").stringConf.createWithDefault("short")


short is defined? Where is the test case? Why this is not documented?

Not sure whether we should introduce a conf here. cc @rxin

Ah yeah this should have been documented, that is a good point. I should have looked more carefully at the configs. The other eventLog configs aren't internal either.

I agree this could also be a boolean, or simply handle anything but "short" or "long" as "short"

gatorsmile · 2018-09-10T06:49:21Z

core/src/main/scala/org/apache/spark/storage/RDDInfo.scala

+    }
    new RDDInfo(rdd.id, rddName, rdd.partitions.length,
-      rdd.getStorageLevel, parentIds, rdd.creationSite.shortForm, rdd.scope)
+      rdd.getStorageLevel, parentIds, callSite, rdd.scope)


This sounds a general issue. Why we only apply it in RDDInfo?

@gatorsmile As far as I am aware, RDDInfo is the only place the call site is included in the event log.

michaelmior · 2018-09-10T11:21:27Z

@gatorsmile @cloud-fan I'll just go with a boolean config as there really is no need for more than two options and this simplifies things quite a bit.

gatorsmile · 2018-09-10T20:54:19Z

@michaelmior Since Spark 2.4 is branch cut, this PR still needs more review. I would revert this PR from branch 2.4 and master first. We can discuss the conf and implementation in the master branch. The preferred conf name is spark.eventLog.callsite.longForm.enabled. Thanks!

srowen

I think it's easy enough to update this to a boolean and add a doc and pick it into branch-2.4; it's not imminent as there are still about 30-40 open issues. Still, it's not vital enough that it must be included.

srowen · 2018-09-10T21:26:46Z

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

    ConfigBuilder("spark.eventLog.overwrite").booleanConf.createWithDefault(false)

+  private[spark] val EVENT_LOG_CALLSITE_FORM =
+    ConfigBuilder("spark.eventLog.callsite").stringConf.createWithDefault("short")


Ah yeah this should have been documented, that is a good point. I should have looked more carefully at the configs. The other eventLog configs aren't internal either.

I agree this could also be a boolean, or simply handle anything but "short" or "long" as "short"

srowen · 2018-09-11T19:49:44Z

Given lack of certainty, and that's this is small and easy to add back in a different form, and the fact that 2.4 is quickly teeing up, let me revert this for now. We can proceed with a different approach in a new PR.

rxin · 2018-09-11T20:01:06Z

Yea we can add this back easily.

…

On Tue, Sep 11, 2018 at 12:50 PM Sean Owen ***@***.***> wrote: Given lack of certainty, and that's this is small and easy to add back in a different form, and the fact that 2.4 is quickly teeing up, let me revert this for now. We can proceed with a different approach in a new PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21433 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATvPPKRRxsg30kJA9RAItGJDHPF4mX_ks5uaBQjgaJpZM4UONdo> .

This is a rework of #21433 to address some concerns there. Closes #22398 from michaelmior/long-callsite2. Authored-by: Michael Mior <mmior@uwaterloo.ca> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit ab25c96) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

This is a rework of #21433 to address some concerns there. Closes #22398 from michaelmior/long-callsite2. Authored-by: Michael Mior <mmior@uwaterloo.ca> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

This is a rework of apache#21433 to address some concerns there. Closes apache#22398 from michaelmior/long-callsite2. Authored-by: Michael Mior <mmior@uwaterloo.ca> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

[SPARK-23820][CORE] Enable use of long form of callsite in logs

245181a

michaelmior force-pushed the long-callsite branch from 9800d2e to 245181a Compare July 2, 2018 19:23

srowen approved these changes Jul 5, 2018

View reviewed changes

asfgit closed this in e58dadb Jul 5, 2018

michaelmior deleted the long-callsite branch July 5, 2018 13:54

gatorsmile reviewed Sep 10, 2018

View reviewed changes

srowen reviewed Sep 10, 2018

View reviewed changes

michaelmior mentioned this pull request Sep 11, 2018

[SPARK-23820][CORE] Enable use of long form of callsite in logs #22398

Closed

Conversation

michaelmior commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiszk commented May 25, 2018

Uh oh!

michaelmior commented May 25, 2018

Uh oh!

srowen commented Jul 2, 2018

Uh oh!

SparkQA commented Jul 2, 2018

Uh oh!

michaelmior commented Jul 2, 2018

Uh oh!

SparkQA commented Jul 3, 2018

Uh oh!

michaelmior commented Jul 3, 2018

Uh oh!

SparkQA commented Jul 4, 2018

Uh oh!

SparkQA commented Jul 4, 2018

Uh oh!

srowen commented Jul 5, 2018

Uh oh!

michaelmior commented Jul 5, 2018

Uh oh!

gatorsmile Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

cloud-fan Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

gatorsmile Sep 10, 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!

gatorsmile Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

srowen Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

gatorsmile Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

michaelmior Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

michaelmior commented Sep 10, 2018

Uh oh!

gatorsmile commented Sep 10, 2018

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

srowen Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

srowen commented Sep 11, 2018

Uh oh!

rxin commented Sep 11, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

michaelmior commented May 25, 2018 •

edited

Loading

gatorsmile Sep 10, 2018 •

edited

Loading