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-49541][BUILD] Upgrade log4j2 to 2.24.1 #48029

Closed
wants to merge 2 commits into from

Conversation

panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Sep 9, 2024

What changes were proposed in this pull request?

The pr aims to upgrade log4j2 from 2.22.1 to 2.24.1.

Why are the changes needed?

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA.

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

No.

@github-actions github-actions bot added the BUILD label Sep 9, 2024
@panbingkun panbingkun changed the title [WIP][SPARK-49541][BUILD] Upgrade log4j2 to 2.24.0 [SPARK-49541][BUILD] Upgrade log4j2 to 2.24.0 Sep 9, 2024
@panbingkun panbingkun changed the title [SPARK-49541][BUILD] Upgrade log4j2 to 2.24.0 [WIP][SPARK-49541][BUILD] Upgrade log4j2 to 2.24.0 Sep 9, 2024
@LuciferYang
Copy link
Contributor

#48057 (comment)

Upon further analysis, it appears that there are some issues with the method copyAndPutAll in org.apache.logging.log4j.internal.map.UnmodifiableArrayBackedMap. From the debugging process, it seems that the intention here is to put executor_id -> 1 into an UnmodifiableArrayBackedMap that already contains data -> some-data. However, the resulting map only contains executor_id -> 1. I believe this is not as expected.

image

image

@LuciferYang
Copy link
Contributor

try to fix this issue: apache/logging-log4j2#2942

@LuciferYang
Copy link
Contributor

try to fix this issue: apache/logging-log4j2#2942

Fixed, let's wait 2.24.1

@LuciferYang
Copy link
Contributor

Testing log4j 2.24.1 #48310

@LuciferYang
Copy link
Contributor

LuciferYang commented Oct 1, 2024

@panbingkun 2.24.1 should be able to pass the tests(https:/LuciferYang/spark/runs/30898064859). Could you please update the PR, including the PR title and description? We can continue to move forward with this PR.

@panbingkun panbingkun changed the title [WIP][SPARK-49541][BUILD] Upgrade log4j2 to 2.24.0 [WIP][SPARK-49541][BUILD] Upgrade log4j2 to 2.24.1 Oct 2, 2024
Copy link
Contributor

@LuciferYang LuciferYang 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

image

But it's best to retrigger the Docker integrated tests :)

@LuciferYang LuciferYang changed the title [WIP][SPARK-49541][BUILD] Upgrade log4j2 to 2.24.1 [SPARK-49541][BUILD] Upgrade log4j2 to 2.24.1 Oct 2, 2024
@panbingkun
Copy link
Contributor Author

+1, LGTM

image But it's best to retrigger the Docker integrated tests :)

Done.

@panbingkun panbingkun marked this pull request as ready for review October 2, 2024 09:45
@LuciferYang
Copy link
Contributor

Merged into master for Spark 4.0. Thanks @panbingkun

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Oct 3, 2024

+1, LGTM.
Thank you, @panbingkun and @LuciferYang .

attilapiros pushed a commit to attilapiros/spark that referenced this pull request Oct 4, 2024
### What changes were proposed in this pull request?
The pr aims to upgrade log4j2 from `2.22.1` to `2.24.1`.

### Why are the changes needed?
- The full release notes:
https:/apache/logging-log4j2/releases/tag/rel%2F2.24.1
https:/apache/logging-log4j2/releases/tag/rel%2F2.24.0
https:/apache/logging-log4j2/releases/tag/rel%2F2.23.1
https:/apache/logging-log4j2/releases/tag/rel%2F2.23.0

- The new version contains some bug fixes:
Fix regression in JdkMapAdapterStringMap performance. (apache/logging-log4j2#2238)
Fix NPE in PatternProcessor for a UNIX_MILLIS pattern (apache/logging-log4j2#2346)
Fix that parameterized message formatting throws an exception when there are insufficient number of parameters. It previously simply
didn't replace the '{}' sequence. The behavior changed in 2.21.0 and should be restored for backward compatibility. (apache/logging-log4j2#2380)
Fix putAll() in the default thread context map implementation (apache/logging-log4j2#2942)

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

### How was this patch tested?
Pass GA.

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

Closes apache#48029 from panbingkun/SPARK-49541.

Authored-by: panbingkun <[email protected]>
Signed-off-by: yangjie01 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants