[SPARK-8981][CORE][FOLLOW-UP] Clean up MDC properties after running a task #28756

Ngone51 · 2020-06-08T15:20:57Z

What changes were proposed in this pull request?

This PR is a followup of #26624. This PR cleans up MDC properties if the original value is empty.
Besides, this PR adds a warning and ignore the value when the user tries to override the value of taskName.

Why are the changes needed?

Before this PR, running the following jobs:

sc.setLocalProperty("mdc.my", "ABC")
sc.parallelize(1 to 100).count()
sc.setLocalProperty("mdc.my", null)
sc.parallelize(1 to 100).count()

there's still MDC value "ABC" in the log of the second count job even if we've unset the value.

Does this PR introduce any user-facing change?

Yes, user will 1) no longer see the MDC values after unsetting the value; 2) see a warning if he/she tries to override the value of taskName.

How was this patch tested?

Tested Manaually.

SparkQA · 2020-06-08T18:15:13Z

Test build #123639 has finished for PR 28756 at commit be3d752.

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

Ngone51 · 2020-06-10T13:29:27Z

ping @cloud-fan @igreenfield Please take a look!

cloud-fan · 2020-06-10T13:55:44Z

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

+      .filter(_._1.startsWith(Executor.MDC_KEY)).map { item =>
        val key = item._1.substring(4)
+        if (key == Executor.TASK_MDC_KEY && item._2 != taskName) {
+          logWarning(s"Override mdc.taskName is not allowed, ignore ${item._2}")


shall we simply set the task mdc key at the end? then it will not be overwritten.

Actually, we've already add the task mdc key at the new line 326. We can remove the warning if it's ok to override silently.

I think the original setMDCForTask method looks OK as long as we set task mdc key at the end. It also helps avoid duplicated code.

Why we do not let override the task name in MDC?

then our document is wrong. We must make sure taskName always represent the value as we documented.

I think we should let the user override it, and write to the log that is overridden.
(windows way is not let you do things, Linux way: with great power comes greater responsibility)

What's the benefit to let users override the task name? It's just confusing to me. Let's not support a non-existing use case.

in the UI you can set the description I think many users can benefit from setting the taskName to be the same as that.

then, they can use custom MDC properties instead?

cloud-fan · 2020-06-10T13:56:30Z

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

    }

    override def run(): Unit = {
+      val oldMdcProperties = mdcProperties.keys.map(k => (k, MDC.get(k)))


Can we simply clear all the mdc properties here?

I think we can normally, and I'd prefer to clear it inside finally.

igreenfield · 2020-06-11T06:32:16Z

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

  val taskDeserializationProps: ThreadLocal[Properties] = new ThreadLocal[Properties]
+
+  val MDC_KEY = "mdc."
+  val TASK_MDC_KEY = s"${MDC_KEY}taskName"


if you change this key you need also to update the docs

IIUC, the change doesn't introduce any difference to the end-user, no?

as I remember before it was just taskName without mdc.taskName

This is used internally in order to handle taskName consistently with custom MDC properties. It has no effect on users.

igreenfield · 2020-06-11T06:33:14Z

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

+      .filter(_._1.startsWith(Executor.MDC_KEY)).map { item =>
        val key = item._1.substring(4)
+        if (key == Executor.TASK_MDC_KEY && item._2 != taskName) {
+          logWarning(s"Override mdc.taskName is not allowed, ignore ${item._2}")


Why we do not let override the task name in MDC?

Ngone51 · 2020-06-11T09:08:22Z

@cloud-fan @igreenfield Have updated according to your comments.

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

SparkQA · 2020-06-11T12:16:31Z

Test build #123842 has finished for PR 28756 at commit 0cefc61.

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

SparkQA · 2020-06-11T13:47:42Z

Test build #123843 has finished for PR 28756 at commit 2f1b86b.

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

cloud-fan · 2020-06-11T14:16:19Z

thanks, merging to master!

Ngone51 · 2020-06-11T15:14:01Z

thanks all!!

melin · 2025-01-08T06:53:20Z

Currently, spark only adds taskName to mdc, can add executorId to MDC as well?
Plan to write logs to kafka via kafka appender, and then periodically write kafka data to s3 for consumption.

https://aws.github.io/aws-emr-containers-best-practices/troubleshooting/docs/where-to-look-for-spark-logs/
Executor Logs - s3://my_s3_log_location/${virtual-cluster-id}/jobs/${job-id}/containers/${spark-application-id}/${spark-job-id-driver-executor-id}/(stderr.gz/stdout.gz)

@Ngone51

fix

fe27f88

probot-autolabeler bot added the CORE label Jun 8, 2020

Ngone51 changed the title ~~[SPARK-8981]~~ [SPARK-8981][CORE][FOLLOW-UP] Clean up MDC properties after running a task Jun 8, 2020

remove setMDC

be3d752

cloud-fan reviewed Jun 10, 2020

View reviewed changes

igreenfield reviewed Jun 11, 2020

View reviewed changes

address comments

0cefc61

cloud-fan reviewed Jun 11, 2020

View reviewed changes

core/src/main/scala/org/apache/spark/executor/Executor.scala Show resolved Hide resolved

clear MDC at the beginning

2f1b86b

cloud-fan approved these changes Jun 11, 2020

View reviewed changes

cloud-fan closed this in 91cd06b Jun 11, 2020

[SPARK-8981][CORE][FOLLOW-UP] Clean up MDC properties after running a task #28756

[SPARK-8981][CORE][FOLLOW-UP] Clean up MDC properties after running a task #28756

Uh oh!

Conversation

Ngone51 commented Jun 8, 2020 • 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!

SparkQA commented Jun 8, 2020

Uh oh!

Ngone51 commented Jun 10, 2020

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!

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!

Ngone51 Jun 11, 2020 • 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!

Ngone51 commented Jun 11, 2020

Uh oh!

Uh oh!

SparkQA commented Jun 11, 2020

Uh oh!

SparkQA commented Jun 11, 2020

Uh oh!

cloud-fan commented Jun 11, 2020

Uh oh!

Ngone51 commented Jun 11, 2020

Uh oh!

melin commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ngone51 commented Jun 8, 2020 •

edited

Loading

Ngone51 Jun 11, 2020 •

edited

Loading

melin commented Jan 8, 2025 •

edited

Loading