Skip to content

Fix async log appender not print error log when bookie starting expectionally#4475

Merged
StevenLuMT merged 2 commits into
apache:masterfrom
AnonHxy:fix_async_log
Feb 17, 2025
Merged

Fix async log appender not print error log when bookie starting expectionally#4475
StevenLuMT merged 2 commits into
apache:masterfrom
AnonHxy:fix_async_log

Conversation

@AnonHxy

@AnonHxy AnonHxy commented Jul 27, 2024

Copy link
Copy Markdown
Contributor

Motivation

Fix #4474

The root cause is that when bookie starting error, the java runtime will exit immediately. And the log4j can not shutdown gracefully:

public static void main(String[] args) {
int retCode = doMain(args);
Runtime.getRuntime().exit(retCode);

Changes

Call LogManager#shutdown in jvm shutdown hook.

@hezhangjian

Copy link
Copy Markdown
Member

IIUC, you mean call LogManager#shutdown will flush the log to the disk?

@AnonHxy AnonHxy changed the title Fix async log appender not print log when bookie starting error Shutdown log before bookie exit to flush async log Jul 28, 2024
@AnonHxy AnonHxy changed the title Shutdown log before bookie exit to flush async log Shutdown log to flush async log before bookie exit Jul 28, 2024

@dlg99 dlg99 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.

What happens if the user doesn't include logs4j's jars and uses logback or something else?
All prod components of BK should only depend on slf4j.

@AnonHxy

AnonHxy commented Jul 28, 2024

Copy link
Copy Markdown
Contributor Author

What happens if the user doesn't include logs4j's jars and uses logback or something else? All prod components of BK should only depend on slf4j.

Good point. But it seems that there is no way to shutdown or force flush the log through slf4j api. So some log will be lost if user using async logger appender.

I think that we could print the key error logs to stderr. So that users can find cause for abnormal exit from the termianl or stderr logs. @dlg99 @shoothzj

@AnonHxy AnonHxy changed the title Shutdown log to flush async log before bookie exit Fix async log appender not print error log when bookie starting expectionally Jul 28, 2024
@AnonHxy

AnonHxy commented Jul 28, 2024

Copy link
Copy Markdown
Contributor Author

rerun failure checks

@hezhangjian

Copy link
Copy Markdown
Member

Thank you for the update. I understand the need to ensure logs are flushed properly. However, adding multiple System.err outputs might not be the most elegant solution. Could we consider pushing for adding this hook to log4j2 instead?

BTW, I am trying to fix the ci in #4476

@StevenLuMT StevenLuMT closed this Feb 15, 2025
@StevenLuMT StevenLuMT reopened this Feb 15, 2025
@StevenLuMT

Copy link
Copy Markdown
Member

reopen's reason: rerun failure checks

@StevenLuMT StevenLuMT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@StevenLuMT StevenLuMT merged commit f3f6956 into apache:master Feb 17, 2025
@lhotari

lhotari commented Feb 17, 2025

Copy link
Copy Markdown
Member

Thank you for the update. I understand the need to ensure logs are flushed properly. However, adding multiple System.err outputs might not be the most elegant solution. Could we consider pushing for adding this hook to log4j2 instead?

When using Log4J2, it would be necessary to call org.apache.logging.log4j.LogManager.shutdown() to ensure that logs get flushed before a forceful JVM exit with Runtime.getRuntime().halt(int).

What happens if the user doesn't include logs4j's jars and uses logback or something else?
All prod components of BK should only depend on slf4j.

In Pulsar, this is handled with reflection:
https://github.com/apache/pulsar/blob/master/pulsar-common/src/main/java/org/apache/pulsar/common/util/ShutdownUtil.java#L27-L40

@hangc0276

hangc0276 commented May 27, 2026

Copy link
Copy Markdown
Contributor

@AnonHxy Would you please check Lari's comment? Just printing System.err.println(e.getMessage()); is not a standard solution.

@hangc0276 hangc0276 added this to the 4.18.0 milestone May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Async log appender not print log when bookie starting error

6 participants