HBASE-26723 Switch to use log4j2.properties file to configure log4j2 #4096
Conversation
This file contains 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. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
apurtell
reviewed
Feb 7, 2022
| if [ -f "${HBASE_HOME}/conf/log4j-hbtop.properties" ] ; then | ||
| HBASE_HBTOP_OPTS="${HBASE_HBTOP_OPTS} -Dlog4j.configuration=file:${HBASE_HOME}/conf/log4j-hbtop.properties" | ||
| if [ -f "${HBASE_HOME}/conf/log4j2-hbtop.properties" ] ; then | ||
| HBASE_HBTOP_OPTS="${HBASE_HBTOP_OPTS} -Dlog4j2.configurationFile=file:${HBASE_HOME}/conf/log4j2-hbtop.properties" |
There was a problem hiding this comment.
Maybe this work will resolve the issue I see starting hbtop with latest branch-2 HEAD
Exception in thread "main" java.lang.NoClassDefFoundError: org/slf4j/bridge/SLF4JBridgeHandler
at org.apache.hadoop.hbase.logging.JulToSlf4jInitializer.<clinit>(JulToSlf4jInitializer.java:36)
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
This can be "fixed" by removing the JUL to SLF4J bridge stuff from default HBASE_OPTS. Seems not appropriate as a generic option for all processes.
I wonder if hbtop should have logging system properties set at all. It is a curses based command line application, not a daemon. If it has anything to log/print, it should go to the console.
There was a problem hiding this comment.
Redirecting the log message all the slf4j should be default I think, and then we could decide whether to actually output it, but changing the log level, appenders, etc.
And I do not think the error here can be fixed by this change, it seems when starting hbtop we do not add some slf4j related jars. Should be a sperated issue.
| @@ -0,0 +1,135 @@ | |||
| #/** | |||
There was a problem hiding this comment.
Why can't we continue to use the existing properties file that ships in HEAD of branch-2? It is because even with the Log4J 1 compatibility stuff we still can't make it compatible with the log4j1 properties files?
For operational compatibility in minor releases, after users upgrade, their existing logging configuration should still work. It's fine to recommend that they make changes, like a switch to the XML format, to gain access to improvements, but it would appear log4j has decided to make arbitrary changes that break existing configurations still, so logging wouldn't work if the user doesn't port.
Here are the first few lines from log4j.properties in branch-2.
# Define some default values that can be overridden by system properties
hbase.root.logger=INFO,console
hbase.security.logger=INFO,console
hbase.log.dir=.
hbase.log.file=hbase.log
hbase.log.level=INFO
# Define the root logger to the system property "hbase.root.logger".
log4j.rootLogger=${hbase.root.logger}
# Logging Threshold
log4j.threshold=ALL
#
# Daily Rolling File Appender
#
log4j.appender.DRFA=org.apache.log4j.DailyRollingFileAppender
log4j.appender.DRFA.File=${hbase.log.dir}/${hbase.log.file}
It looks like the new properties file format drops the "log4j" prefix? Can we keep it?
Or, we could implement logic in the launch script that looks at the user's existing log4j properties file and rewrites it, and uses the rewritten version, if it detects this?
Or we could intercept the load of the properties file and do some simple string frobbing to handle this case?
There was a problem hiding this comment.
This is log4j2.properties, not log4j.properties. Users could keep their log4j.properties and it will be handled by log4j-1.2-api. Of course if you have some customized appenders then there will be problems.
The log4j2 uses a different syntax, so if users want to fully upgrade to log4j2 then they need to change their configuration ways. But if you want to have the ability to config level and appenders at once, properties file is the only possible way. For other configuration format, such as xml or yaml, there is no way to configuration them at once.
|
🎊 +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. |
|
💔 -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. |
|
2.17.2 is finally out. Let me modify the PR. |
|
🎊 +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. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
The checkstyle issue is the lines in log4j2.properties file are longer than 100. I do not think this is very critical. So any other concerns here? After landing this on master, we could see how to better backport it to branch-2.5+. Thanks. |
apurtell
approved these changes
Feb 28, 2022
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.
No description provided.