Ensure logging is configured for CLI commands #27523
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
jasontedor
added
:Core/Infra/Core
Any CLI commands that depend on core Elasticsearch might touch classes (directly or indirectly) that depends on logging. If they do this and logging is not configured, Log4j will dump status error messages to the console. As such, we need to ensure that any such CLI command configures logging (with a trivial configuration that dumps log messages to the console). Previously we did this in the base CLI command but with the refactoring of this class out of core Elasticsearch, we no longer configure logging there (since we did not want this class to depend on settings and logging). However, this meant for some CLI commands (like the plugin CLI) we were no longer configuring logging. This commit adds base classes between the low-level command and multi-command classes that ensure that logging is configured. Any CLI command that depends on core Elasticsearch should use this infrastructure to ensure logging is configured. There is one exception to this: Elasticsearch itself because it takes reponsibility into its own hands for configuring logging from Elasticsearch settings and log4j2.properties. We preserve this special status.
jasontedor
force-pushed
the
cli-logging
branch
from
439a503 to
97482df
Compare
* master: Revert "Adjust CombinedDeletionPolicy for multiple commits (elastic#27456)" Transpose expected and actual, and remove duplicate info from message. (elastic#27515) [DOCS] Fixed broken link in breaking changes Backport wait_for_initialiazing_shards to cluster health API
tlrx
approved these changes
Nov 25, 2017
|
|
||
| /** | ||
| * Configures logging without Elasticsearch configuration files based on the system property "es.logger.level" only. As such, any | ||
| * logging will written to the console. |
There was a problem hiding this comment.
Thanks. I pushed 31477c2.
jasontedor
added a commit
that referenced
this pull request
Nov 25, 2017
Any CLI commands that depend on core Elasticsearch might touch classes (directly or indirectly) that depends on logging. If they do this and logging is not configured, Log4j will dump status error messages to the console. As such, we need to ensure that any such CLI command configures logging (with a trivial configuration that dumps log messages to the console). Previously we did this in the base CLI command but with the refactoring of this class out of core Elasticsearch, we no longer configure logging there (since we did not want this class to depend on settings and logging). However, this meant for some CLI commands (like the plugin CLI) we were no longer configuring logging. This commit adds base classes between the low-level command and multi-command classes that ensure that logging is configured. Any CLI command that depends on core Elasticsearch should use this infrastructure to ensure logging is configured. There is one exception to this: Elasticsearch itself because it takes reponsibility into its own hands for configuring logging from Elasticsearch settings and log4j2.properties. We preserve this special status. Relates #27523
jasontedor
added a commit
that referenced
this pull request
Nov 25, 2017
Any CLI commands that depend on core Elasticsearch might touch classes (directly or indirectly) that depends on logging. If they do this and logging is not configured, Log4j will dump status error messages to the console. As such, we need to ensure that any such CLI command configures logging (with a trivial configuration that dumps log messages to the console). Previously we did this in the base CLI command but with the refactoring of this class out of core Elasticsearch, we no longer configure logging there (since we did not want this class to depend on settings and logging). However, this meant for some CLI commands (like the plugin CLI) we were no longer configuring logging. This commit adds base classes between the low-level command and multi-command classes that ensure that logging is configured. Any CLI command that depends on core Elasticsearch should use this infrastructure to ensure logging is configured. There is one exception to this: Elasticsearch itself because it takes reponsibility into its own hands for configuring logging from Elasticsearch settings and log4j2.properties. We preserve this special status. Relates #27523
|
Thanks @tlrx. |
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.
Any CLI commands that depend on core Elasticsearch might touch classes (directly or indirectly) that depends on logging. If they do this and logging is not configured, Log4j will dump status error messages to the console. As such, we need to ensure that any such CLI command configures logging (with a trivial configuration that dumps log messages to the console). Previously we did this in the base CLI command but with the refactoring of this class out of core Elasticsearch, we no longer configure logging there (since we did not want this class to depend on settings and logging). However, this meant for some CLI commands (like the plugin CLI) we were no longer configuring logging. This commit adds base classes between the low-level command and multi-command classes that ensure that logging is configured. Any CLI command that depends on core Elasticsearch should use this infrastructure to ensure logging is configured. There is one exception to this: Elasticsearch itself because it takes reponsibility into its own hands for configuring logging from Elasticsearch settings and log4j2.properties. We preserve this special status.
Closes #27521