HADOOP-18631. Migrate Async appenders to log4j properties #5418
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
Tests are passing locally. Added more logs to check more on why they are failing on Jenkins build. |
|
@jojochuang could you please take a look in the meantime? |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Feb 22, 2023
...roject/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/MetricsLoggerTask.java
Show resolved
Hide resolved
|
|
||
| # TODO : log4j2 properties to provide example for using Async appender with other appenders | ||
| hdfs.audit.logger=INFO,ASYNCAPPENDER,RFAAUDIT |
There was a problem hiding this comment.
I can not recall how to config log4j1 AsyncAppender, but I believe we need to use different append ref for datanode and namenode? At least they need to log to different log files?
There was a problem hiding this comment.
~~ As far as AsyncAppender is concerned, we can use same ref. It's the other appenders that make things different if we want to include them.
For this example, hdfs.audit.logger is basically used only for FSNamesystem audit logs. ~~
There was a problem hiding this comment.
Oh sorry, please ignore above comment. hdfs.audit.logger is only meant to be used for namenode if that was the question (datanode vs namenode log files).
| log4j.appender.RFAAUDIT.MaxFileSize=${hdfs.audit.log.maxfilesize} | ||
| log4j.appender.RFAAUDIT.MaxBackupIndex=${hdfs.audit.log.maxbackupindex} | ||
| log4j.appender.ASYNCAPPENDER=org.apache.log4j.AsyncAppender |
There was a problem hiding this comment.
AsyncAppender itself supports logging to file? Or we need to let it wrap another appender?
There was a problem hiding this comment.
It turns out both log4j1 and log4j2 allows wrapping other appenders within AsyncAppender. However, log4j1 supports this through xml only, whereas log4j2 supports it using both xml as well as properties.
log4j1 AsyncAppender javadoc:
Important note: The AsyncAppender can only be script configured using the DOMConfigurator.
Hence I was thinking, instead of creating a new custom appender that eventually wraps other appender with AsyncAppender and make this overall log4j2 migration even more painful, we can wait until we replace log4j.properties with log4j2.properties and then we can configure AsyncAppender to wrap RFA, I have already tested this out locally.
btw this properties file is used for test purpose only.
virajjasani
commented
Feb 22, 2023
Comment on lines
28
to
34
| /** | ||
| * Until we migrate to log4j2, use this appender for namenode audit logger as well as | ||
| * datanode and namenode metric loggers with log4j1 properties. | ||
| * This appender will take parameters necessary to supply RollingFileAppender to AsyncAppender. | ||
| * While migrating to log4j2, we can directly specify appender ref to Async appender. | ||
| */ | ||
| public class AsyncRFAAppender extends AsyncAppender { |
There was a problem hiding this comment.
With this patch, even though we have new custom appender, it's only temporary here.
This solves two purposes:
- When we migrate from log4j to log4j2 properties, we can directly use log4j2 Async appender and provide appender ref as other appenders (i.e. wrap RFA appender to Async appender). Hence, this custom appender no longer needs to be migrated to log4j2, rather we can remove it cleanly.
- Before migrating to log4j2, we still have a way to wrap RFA in Async appender without touching the code. We can directly refer to this custom appender in log4j.properties (as done with this patch)
It's temporary but necessary until going to log4j2, so that we don't loose functionality of wrapping RFA in Async appender.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Will fix the spotbugs warning |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
jojochuang
approved these changes
Feb 25, 2023
|
There is a test failing Didn't check this and related changes. But looks like same path is used for Please raise an addendum and fix the test if it is indeed related to this. Will leave it further to @jojochuang.... |
|
This flaky behavior is relevant to this change, even though it never failed on any of above build runs. Let me create an addendum. Thanks |
|
Addendum PR #5451 |
ferdelyi
pushed a commit
to ferdelyi/hadoop
that referenced
this pull request
May 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sub-task of HADOOP-16206 (Migrate from Log4j1 to Log4j2)
A short summary: