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

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

Merged
merged 2 commits into from
Feb 17, 2025

Conversation

AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented Jul 27, 2024

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
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
Copy link
Contributor

@dlg99 dlg99 left a comment

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
Copy link
Contributor Author

AnonHxy commented Jul 28, 2024

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
Copy link
Contributor Author

AnonHxy commented Jul 28, 2024

rerun failure checks

@hezhangjian
Copy link
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
Member

reopen's reason: rerun failure checks

Copy link
Member

@StevenLuMT StevenLuMT left a comment

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
45 of 49 checks passed
@lhotari
Copy link
Member

lhotari commented Feb 17, 2025

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

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
5 participants