Skip to content

[CELEBORN-1994] Introduce disruptor dependency to support asynchronous logging of log4j2#3246

Closed
SteNicholas wants to merge 2 commits into
apache:mainfrom
SteNicholas:CELEBORN-1994
Closed

[CELEBORN-1994] Introduce disruptor dependency to support asynchronous logging of log4j2#3246
SteNicholas wants to merge 2 commits into
apache:mainfrom
SteNicholas:CELEBORN-1994

Conversation

@SteNicholas

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Introduce disruptor dependency to support asynchronous logging of log4j2.

Why are the changes needed?

We add -Dlog4j2.contextSelector=org.apache.logging.log4j.core.async.AsyncLoggerContextSelector in CELEBORN_MASTER_JAVA_OPTS and CELEBORN_WOKRER_JAVA_OPTS for production environment. AsyncLoggerContextSelector depends on disruptor dependency. Therefore, it's recommend to introduce disruptor dependency to support log4j2 asynchronous loggers.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Cluster test.

@SteNicholas

Copy link
Copy Markdown
Member Author

Ping @pan3793.

@SteNicholas SteNicholas requested a review from Copilot May 7, 2025 12:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (5)
  • dev/deps/dependencies-server: Language not supported
  • master/pom.xml: Language not supported
  • pom.xml: Language not supported
  • project/CelebornBuild.scala: Language not supported
  • worker/pom.xml: Language not supported

@FMX

FMX commented May 12, 2025

Copy link
Copy Markdown
Contributor

According to the log4j docs(https://logging.apache.org/log4j/2.x/manual/async.html), is there a statistically significant difference between asynchronous and synchronous logging solutions?

@pan3793

pan3793 commented May 12, 2025

Copy link
Copy Markdown
Member

log4j2 async logger is generally faster than the default, but don't forget to update LICENSE/NOTICE when pulling new deps

@SteNicholas SteNicholas requested review from FMX and pan3793 May 12, 2025 06:13
@SteNicholas

Copy link
Copy Markdown
Member Author

@pan3793, I have checked the LICENSE file of disruptor and added the dependency into LICENSE-binary file.
@FMX, the asynchronous logger does not impact on shuffle execution.

@SteNicholas

Copy link
Copy Markdown
Member Author

Ping @FMX, @RexXiong.

@FMX FMX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks. Merged into main(v0.6.0).

@FMX FMX closed this in 8e66ac8 May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants