Plugin cli tool should not create empty log files #13933
Merged
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
| Environment env = InternalSettingsPreparer.prepareEnvironment(EMPTY, Terminal.DEFAULT); | ||
| LogConfigurator.configure(env.settings()); | ||
| // initialize default for es.logger.level because we will not read the logging.yml | ||
| String loggerLevel = System.getProperty("es.logger.level"); |
There was a problem hiding this comment.
System.getProperty("es.logger.level", "INFO");
brwe
added
v2.1.0
v2.0.0
v2.2.0
>bug
:Core/Infra/Logging
| configure(settings, true); | ||
| } | ||
|
|
||
| public static void configure(Settings settings, boolean resolveConfig) { |
There was a problem hiding this comment.
Probably deserves javadoc saying that resolveConfig controls if the config file is read or not.
|
left some minor comments, agreeing with the others, but this looks good. thx for digging! |
brwe
force-pushed
the
plugin-cli-logging
branch
from
e0975b1 to
0317de1
Compare
|
@dadoonet @nik9000 @spinscale thanks a lot for the review! I addressed all comments. |
| @@ -83,15 +84,24 @@ | |||
| .put("xml", "org.apache.log4j.XMLLayout") | |||
| .immutableMap(); | |||
|
|
|||
| public static void configure(Settings settings) { | |||
| /** | |||
| * Consolidates settings and converts them into actual log4j settings, then initializes loggers and appenders. | |||
There was a problem hiding this comment.
Thanks for writing this. Wonderful.
|
LGTM |
brwe
force-pushed
the
plugin-cli-logging
branch
from
208e748 to
3e4976d
Compare
Plugin cli tools configures logging with whatever is in the logging.yml. If a file appender is configured for any of the logs this will cause creation of an empty log file. If a plugin was for example installed as root it will create empty logs at es.home/logs. This is problematic when for example plugins are installed as root and es is run as service. Logs will then be created in /usr/share/elasticsearch/logs and can later not be removed by for example dpkg -r or -purge. To avoid this, configure the logger to use an appender that writes to the same output that plugin cli tool does. This allows other components that are called from Plugin cli tool to write to the same terminal that plugin cli tool writes to by using the logging mechanism already in place. The logging conf is not read at all pb plugin cli tool. As a side effect, the loging level for components that are called from the plugin command such as the jar hell check can now be configured with -Des.logger.level which makes it easier to debug the jar hell check.
brwe
force-pushed
the
plugin-cli-logging
branch
from
3e4976d to
9492be6
Compare
brwe
added a commit
that referenced
this pull request
Oct 6, 2015
plugin cli tool should not create empty log files
clintongormley
changed the title
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.
Plugin cli tools configures logging with whatever is in the logging.yml.
If a file appender is configured for any of the logs this will cause creation
of an empty log file. If a plugin was for example installed as root it will
create empty logs at es.home/logs.
This is problematic when for example plugins are installed as root and es is run
as service. Logs will then be created in /usr/share/elasticsearch/logs
and can later not be removed by for example dpkg -r or -purge.
To avoid this, configure the logger to use an appender that writes to the same
output that plugin cli tool does. This allows other components that are called
from Plugin cli tool to write to the same terminal that plugin cli tool writes to
by using the logging mechanism already in place.
The logging conf is not read at all pb plugin cli tool.
As a side effect, the loging level for components that are called
from the plugin command such as the jar hell check can now be configured
with -Des.logger.level which makes it easier to debug the jar hell check.