-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Allow multiple consoleAppender to be used in peon logging #14521
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One minor comment
} | ||
} | ||
|
||
@Nullable | ||
private Appender findConsoleAppender() | ||
@NotNull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be @Nonnull
rather than @NotNull
as there's no validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Thanks
CC: @suneet-s @TSFenwick |
288f141
to
b180154
Compare
When testing this change without specifying a Console appender, I see this in the task logs.
I know this is not part of this change, as I have seen this error before this patch. Since you are changing the code, could you look into why the injected console appender is not started |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on the approach, thank you!
This error is not introduced by my change. The error is due to the created |
* Allow multiple consoleAppender to be used in peon logging * Fix Attempted to append to non-started appender error
Fixes ConsoleLoggingEnforcementConfigurationFactory would still enforce an additional ConsoleAppender if there are multiple ConsoleAppender defined
Description
The PR #14094 allow for multiple/existing log4j appenders to be kept when there is a ConsoleAppender configured for a Logger. However, this doesn't work as expected when there are multiple ConsoleAppender defined. The function ConsoleLoggingEnforcementConfigurationFactory#findConsoleAppender currently only considered the first ConsoleAppender it finds and verify that this ConsoleAppender is set in the Loggers. However, this fails to consider that multiple ConsoleAppenders could be defined. When we ensure there is a console logger defined for each Logger (in the function ConsoleLoggingEnforcementConfigurationFactory#applyConsoleAppender), we should make it such that ANY of the defined ConsoleAppender is sufficient (not just the first ConsoleAppender in the list)
This PR has: