[SPARK-31970][CORE] Make MDC configuration step be consistent between setLocalProperty and log4j.properties #28801

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

What changes were proposed in this pull request?

This PR proposes to use "mdc.XXX" as the consistent key for both sc.setLocalProperty and log4j.properties when setting up configurations for MDC.

Why are the changes needed?

It's weird that we use "mdc.XXX" as key to set MDC value via sc.setLocalProperty while we use "XXX" as key to set MDC pattern in log4j.properties. It could also bring extra burden to the user.

Does this PR introduce any user-facing change?

No, as MDC feature is added in version 3.1, which hasn't been released.

How was this patch tested?

Tested manually.

Ngone51 · 2020-06-11T15:50:21Z

cc @cloud-fan @igreenfield Please take a look, thanks!

cloud-fan · 2020-06-11T15:56:04Z

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

  // task is fully deserialized. When possible, the TaskContext.getLocalProperty call should be
  // used instead.
  val taskDeserializationProps: ThreadLocal[Properties] = new ThreadLocal[Properties]
+  val MDC = "mdc."


this is too short... I'm ok to just hardcode it in the code.

cloud-fan · 2020-06-11T15:57:18Z

I don't have a strong preference of it, seems OK. cc @jiangxb1987

igreenfield · 2020-06-11T17:05:37Z

seems ok to me

jiangxb1987

LGTM

dongjoon-hyun · 2020-06-11T17:35:45Z

Hi, All. Sorry, but how can we disable this MDC feature and enforce to generate the same result like Spark 2.4.6?

SparkQA · 2020-06-11T19:36:12Z

Test build #123858 has finished for PR 28801 at commit b8affa3.

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

Ngone51 · 2020-06-12T01:58:02Z

Hi, All. Sorry, but how can we disable this MDC feature and enforce to generate the same result like Spark 2.4.6?

@dongjoon-hyun

As for end-user, they will not see the MDC properties in the log if they use default log4j.peroperties (since MDC requires extra pattern configuration). So it's still the same compares to Spark 2.4.6.

But internally, yes, Spark will always add at least one MDC property (taskName) to the MDC, even if nowhere else will actually use it. But considering the data is small, so probably it's fine to do it without a controllable flag.

cloud-fan · 2020-06-12T04:45:08Z

yea the log4j property file is kind of the config to control this MDC feature, and it's off by default.

SparkQA · 2020-06-12T07:05:02Z

Test build #123884 has finished for PR 28801 at commit 2c77bbb.

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

cloud-fan · 2020-06-12T09:46:55Z

retest this please

SparkQA · 2020-06-12T13:16:27Z

Test build #123916 has finished for PR 28801 at commit 2c77bbb.

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

cloud-fan · 2020-06-12T13:39:11Z

retest this please

SparkQA · 2020-06-12T16:16:27Z

Test build #123930 has finished for PR 28801 at commit 2c77bbb.

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

Ngone51 · 2020-06-12T16:57:18Z

retest this please.

SparkQA · 2020-06-12T19:50:02Z

Test build #123939 has finished for PR 28801 at commit 2c77bbb.

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

jiangxb1987 · 2020-06-12T23:22:47Z

retest this please

SparkQA · 2020-06-13T02:22:58Z

Test build #123947 has finished for PR 28801 at commit 2c77bbb.

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

dongjoon-hyun

+1, LGTM. Thank you, @Ngone51 , @cloud-fan , @jiangxb1987 .
Merged to master.

Ngone51 · 2020-06-15T05:33:52Z

thanks all!!

### What changes were proposed in this pull request? Currently there are two MDC keys for task name: * `mdc.taskName`, which is introduced in #28801. Before the change, it was `taskName`. * `task_name`: introduce from the structured logging framework project. To make the MDC keys unified, this PR renames the `mdc.taskName` as `task_name`. This MDC is showing frequently in logs when running Spark application. Before change: ``` "context":{"mdc.taskName":"task 19.0 in stage 0.0 (TID 19)”} ``` after change ``` "context":{“task_name":"task 19.0 in stage 0.0 (TID 19)”} ``` ### Why are the changes needed? 1. Make the MDC names consistent 2. Minor upside: this will allow users to query task names with `SELECT * FROM logs where context.task_name = ...`. Otherwise, querying with `context.mdc.task_name` will result in an analysis exception. Users will have to query with `context['mdc.task_name']` ### Does this PR introduce _any_ user-facing change? No really. The MDC key is used by developers for debugging purpose. ### How was this patch tested? Manual test ### Was this patch authored or co-authored using generative AI tooling? No Closes #46386 from gengliangwang/unify. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

### What changes were proposed in this pull request? Currently there are two MDC keys for task name: * `mdc.taskName`, which is introduced in apache#28801. Before the change, it was `taskName`. * `task_name`: introduce from the structured logging framework project. To make the MDC keys unified, this PR renames the `mdc.taskName` as `task_name`. This MDC is showing frequently in logs when running Spark application. Before change: ``` "context":{"mdc.taskName":"task 19.0 in stage 0.0 (TID 19)”} ``` after change ``` "context":{“task_name":"task 19.0 in stage 0.0 (TID 19)”} ``` ### Why are the changes needed? 1. Make the MDC names consistent 2. Minor upside: this will allow users to query task names with `SELECT * FROM logs where context.task_name = ...`. Otherwise, querying with `context.mdc.task_name` will result in an analysis exception. Users will have to query with `context['mdc.task_name']` ### Does this PR introduce _any_ user-facing change? No really. The MDC key is used by developers for debugging purpose. ### How was this patch tested? Manual test ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#46386 from gengliangwang/unify. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

impr

b8affa3

probot-autolabeler bot added CORE DOCS labels Jun 11, 2020

Ngone51 changed the title ~~[SPARK-31970][CORE] Make MDC configuration step be consistent between setLocalProperty and log4j~~ [SPARK-31970][CORE] Make MDC configuration step be consistent between setLocalProperty and log4j.properties Jun 11, 2020

cloud-fan reviewed Jun 11, 2020

View reviewed changes

jiangxb1987 approved these changes Jun 11, 2020

View reviewed changes

fix

2c77bbb

dongjoon-hyun approved these changes Jun 14, 2020

View reviewed changes

dongjoon-hyun closed this in 54e702c Jun 14, 2020

igreenfield mentioned this pull request Feb 2, 2021

[SPARK-8981][CORE][test-hadoop3.2][test-java11] Add MDC support in Executor #26624

Closed

gengliangwang mentioned this pull request May 4, 2024

[SPARK-48131][Core] Unify MDC key mdc.taskName and task_name #46386

Closed

[SPARK-31970][CORE] Make MDC configuration step be consistent between setLocalProperty and log4j.properties #28801

[SPARK-31970][CORE] Make MDC configuration step be consistent between setLocalProperty and log4j.properties #28801

Uh oh!

Conversation

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

Ngone51 commented Jun 11, 2020

Uh oh!

cloud-fan Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

Ngone51 Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jun 11, 2020

Uh oh!

igreenfield commented Jun 11, 2020

Uh oh!

jiangxb1987 left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jun 11, 2020

Uh oh!

SparkQA commented Jun 11, 2020

Uh oh!

Ngone51 commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

cloud-fan commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

cloud-fan commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

Ngone51 commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

jiangxb1987 commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 13, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Ngone51 commented Jun 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Ngone51 commented Jun 11, 2020 •

edited

Loading

Ngone51 commented Jun 12, 2020 •

edited

Loading