Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-48126][Core] Make spark.log.structuredLogging.enabled effective #46452

Closed
wants to merge 3 commits into from

Conversation

gengliangwang
Copy link
Member

@gengliangwang gengliangwang commented May 7, 2024

What changes were proposed in this pull request?

Currently, the spark conf spark.log.structuredLogging.enabled is not taking effect. The current code base checks this config in the method prepareSubmitEnvironment. However, Log4j is already initialized before that.

This PR is to fix it by checking the config spark.log.structuredLogging.enabled before the initialization of Log4j.
Also, this PR enhances the doc for this configuration.

Why are the changes needed?

Bug fix. After the fix, the Spark conf spark.log.structuredLogging.enabled takes effect.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Manual test.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: GPT-4
I used GPT-4 to improve the documents.

@gengliangwang
Copy link
Member Author

cc @panbingkun

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR doesn't have any test coverage, could you elaborate more specifically in the PR description when this doesn't work?

Currently, the spark conf spark.log.structuredLogging.enabled is not taking effect.

@gengliangwang
Copy link
Member Author

Since this PR doesn't have any test coverage, could you elaborate more specifically in the PR description when this doesn't work?

You are right. I just updated the description.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM for 4.0.0-preview.

@gengliangwang
Copy link
Member Author

@dongjoon-hyun @HyukjinKwon Thanks for the review. I am merging this one to master

JacobZheng0927 pushed a commit to JacobZheng0927/spark that referenced this pull request May 11, 2024
### What changes were proposed in this pull request?

Currently, the spark conf `spark.log.structuredLogging.enabled` is not taking effect. The current code base checks this config in the method `prepareSubmitEnvironment`. However, Log4j is already initialized before that.

This PR is to fix it by checking the config `spark.log.structuredLogging.enabled` before the initialization of Log4j.
Also, this PR enhances the doc for this configuration.

### Why are the changes needed?

Bug fix. After the fix, the Spark conf `spark.log.structuredLogging.enabled` takes effect.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

Manual test.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: GPT-4
I used GPT-4 to improve the documents.

Closes apache#46452 from gengliangwang/makeConfEffective.

Authored-by: Gengliang Wang <gengliang@apache.org>
Signed-off-by: Gengliang Wang <gengliang@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants