You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by srowen <gi...@git.apache.org> on 2014/07/23 16:00:09 UTC
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
GitHub user srowen opened a pull request:
https://github.com/apache/spark/pull/1547
SPARK-2646. log4j initialization not quite compatible with log4j 2.x
The logging code that handles log4j initialization leads to an stack overflow error when used with log4j 2.x, which has just been released. This occurs even a downstream project has correctly adjusted SLF4J bindings, and that is the right thing to do for log4j 2.x, since it is effectively a separate project from 1.x.
Here is the relevant bit of Logging.scala:
```
private def initializeLogging() {
// If Log4j is being used, but is not initialized, load a default properties file
val binder = StaticLoggerBinder.getSingleton
val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
if (!log4jInitialized && usingLog4j) {
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
log.info(s"Using Spark's default log4j profile: $defaultLogProps")
case None =>
System.err.println(s"Spark was unable to load $defaultLogProps")
}
}
Logging.initialized = true
// Force a call into slf4j to initialize it. Avoids this happening from mutliple threads
// and triggering this: http://mailman.qos.ch/pipermail/slf4j-dev/2010-April/002956.html
log
}
```
The first minor issue is that there is a call to a logger inside this method, which is initializing logging. In this situation, it ends up causing the initialization to be called recursively until the stack overflow. It would be slightly tidier to log this only after Logging.initialized = true. Or not at all. But it's not the root problem, or else, it would not work at all now.
The calls to log4j classes here always reference log4j 1.2 no matter what. For example, there is not getAllAppenders in log4j 2.x. That's fine. Really, "usingLog4j" means "using log4j 1.2" and "log4jInitialized" means "log4j 1.2 is initialized".
usingLog4j should be false for log4j 2.x, because the initialization only matters for log4j 1.2. But, it's true, and that's the real issue. And log4jInitialized is always false, since calls to the log4j 1.2 API are stubs and no-ops in this setup, where the caller has swapped in log4j 2.x. Hence the loop.
This is fixed, I believe, if "usingLog4j" can be false for log4j 2.x. The SLF4J static binding class has the same name for both versions, unfortunately, which causes the issue. However they're in different packages. For example, if the test included "... and begins with org.slf4j", it should work, as the SLF4J binding for log4j 2.x is provided by log4j 2.x at the moment, and is in package org.apache.logging.slf4j.
Of course, I assume that SLF4J will eventually offer its own binding. I hope to goodness they at least name the binding class differently, or else this will again not work. But then some other check can probably be made.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/srowen/spark SPARK-2646
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/1547.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1547
----
commit 6f3c1d3269f0e3083aea538fb21cb50fbefe4588
Author: Sean Owen <sr...@gmail.com>
Date: 2014-07-23T13:59:10Z
Remove log statement in logging initialization, and distinguish log4j 1.2 from 2.0, to avoid stack overflow in initialization
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50608797
QA results for PR 1547:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17445/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50604815
QA tests have started for PR 1547. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17445/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50663751
QA tests have started for PR 1547. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17466/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15570127
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +110,26 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This minimally distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 =
+ binderClass.endsWith("Log4jLoggerFactory") && binderClass.startsWith("org.slf4j.")
+ val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
+ if (!log4j12Initialized && usingLog4j12) {
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
- log.info(s"Using Spark's default log4j profile: $defaultLogProps")
--- End diff --
Ah I see - I'm somewhat confused why this ever worked at all. I think it's okay to make it a print statement to stderr, so it shows up in the same output stream as our default log.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15510944
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +110,26 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This minimally distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 =
+ binderClass.endsWith("Log4jLoggerFactory") && binderClass.startsWith("org.slf4j.")
+ val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
+ if (!log4j12Initialized && usingLog4j12) {
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
- log.info(s"Using Spark's default log4j profile: $defaultLogProps")
--- End diff --
any reason to ditch this log statement?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15631493
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +107,27 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // Don't use a logger in here, as this is itself occurring during initialization of a logger
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 = "org.slf4j.impl.Log4jLoggerFactory".equals(binderClass)
+ val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
+ if (!log4j12Initialized && usingLog4j12) {
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
- log.info(s"Using Spark's default log4j profile: $defaultLogProps")
+ System.out.println(s"Using Spark's default log4j profile: $defaultLogProps")
--- End diff --
I will change it. `System.err` felt inappropriate for informational messages (but not for errors -- if you can't configure the logger, where else can you turn? so the following `System.err` makes sense). Yeah I'm not sure `System.err` is a great place to log by default, but people can change it. Here you couldn't move this informational message out of `System.err` no matter what. That was my thinking but I imagine this is not a big deal.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15599021
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +110,26 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This minimally distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 =
+ binderClass.endsWith("Log4jLoggerFactory") && binderClass.startsWith("org.slf4j.")
+ val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
+ if (!log4j12Initialized && usingLog4j12) {
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
- log.info(s"Using Spark's default log4j profile: $defaultLogProps")
--- End diff --
My concern there is just polluting stderr with a single informational log message. It could set a flag that causes the log statement at the end of the message if it's important?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50738443
QA results for PR 1547:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17574/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15510992
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +110,26 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This minimally distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 =
+ binderClass.endsWith("Log4jLoggerFactory") && binderClass.startsWith("org.slf4j.")
--- End diff --
just wondering - what is the advantage here over just checking for `org.slf4j.impl.Log4jLoggerFactory`? I think this class name is table in all 1.X releases... is the concern to guard against them changing it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50733900
QA tests have started for PR 1547. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17574/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15576553
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +110,26 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This minimally distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 =
+ binderClass.endsWith("Log4jLoggerFactory") && binderClass.startsWith("org.slf4j.")
+ val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
+ if (!log4j12Initialized && usingLog4j12) {
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
- log.info(s"Using Spark's default log4j profile: $defaultLogProps")
--- End diff --
It worked because the line just preceding it actually dig configure log4j 1.2. `log4j12Initialized` became true in the next recursive call, although the check for `Logging.initialized` is still false. However with log4j 2.x, `log4j12Initialized` will not become `true` as a result of this call to `PropertyConfigurator`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50584544
One other thing, there is some code in this file that could be simplified to this - not necessary for merging this, but I noticed it on review:
```
// Ignore trailing $'s in the class names for Scala objects
className.stripSuffix("$")
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-49892024
QA results for PR 1547:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17037/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50670567
QA results for PR 1547:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17466/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15511267
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +110,26 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This minimally distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 =
+ binderClass.endsWith("Log4jLoggerFactory") && binderClass.startsWith("org.slf4j.")
--- End diff --
The check was initially just for `Log4jLoggerFactory`. I suppose that check would survive package changes in SLF4J. I wanted to minimize the impact, so am checking for "org.slf4j.*Log4jLoggerFactory" now. I would be completely fine with a simple string comparison too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15511218
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +110,26 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This minimally distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 =
+ binderClass.endsWith("Log4jLoggerFactory") && binderClass.startsWith("org.slf4j.")
+ val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
+ if (!log4j12Initialized && usingLog4j12) {
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
- log.info(s"Using Spark's default log4j profile: $defaultLogProps")
--- End diff --
See description -- basically a logger is being called while logging is being initialized, and that seems a bit dicey. That was the very first symptom of this issue, the infinite recursion. It seemed better to avoid. I could also make it a `System.out` in this special circumstance, or, record a flag and log it after init is clearly complete.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50599689
No problem, all done, including the stripSuffix change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/1547
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15569831
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +110,26 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This minimally distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 =
+ binderClass.endsWith("Log4jLoggerFactory") && binderClass.startsWith("org.slf4j.")
--- End diff --
For the benefit of simplicity and clarity to future developers, I'd prefer the simpler string comparison. But it's not a strong opinion of you have strong feelings on it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50603304
QA results for PR 1547:<br>- This patch FAILED unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17442/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15598789
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +110,26 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This minimally distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 =
+ binderClass.endsWith("Log4jLoggerFactory") && binderClass.startsWith("org.slf4j.")
+ val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
+ if (!log4j12Initialized && usingLog4j12) {
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
- log.info(s"Using Spark's default log4j profile: $defaultLogProps")
--- End diff --
Ah I see. In any case, are you okay to add a print statement to stderr?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50600089
QA tests have started for PR 1547. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17442/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15623952
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +107,27 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // Don't use a logger in here, as this is itself occurring during initialization of a logger
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 = "org.slf4j.impl.Log4jLoggerFactory".equals(binderClass)
+ val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
+ if (!log4j12Initialized && usingLog4j12) {
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
- log.info(s"Using Spark's default log4j profile: $defaultLogProps")
+ System.out.println(s"Using Spark's default log4j profile: $defaultLogProps")
--- End diff --
Any reason not to go to System.err? This is where the default spark logs will go (i.e if the user is in this code path) - so it would be consistent with that. We also bail out to System.err just a line below this as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15604577
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +110,26 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This minimally distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 =
+ binderClass.endsWith("Log4jLoggerFactory") && binderClass.startsWith("org.slf4j.")
+ val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
+ if (!log4j12Initialized && usingLog4j12) {
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
- log.info(s"Using Spark's default log4j profile: $defaultLogProps")
--- End diff --
On second thought, I got more paranoid the more I reasoned about it. There would still be a reentrant call through a synchronized block depending on state of the initialized flag... just didn't seem worth it. I made it a System.out message and added a comment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50806638
Thanks @srowen - looks good to me!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/1547#discussion_r15603354
--- Diff: core/src/main/scala/org/apache/spark/Logging.scala ---
@@ -110,23 +110,26 @@ trait Logging {
}
private def initializeLogging() {
- // If Log4j is being used, but is not initialized, load a default properties file
- val binder = StaticLoggerBinder.getSingleton
- val usingLog4j = binder.getLoggerFactoryClassStr.endsWith("Log4jLoggerFactory")
- val log4jInitialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
- if (!log4jInitialized && usingLog4j) {
+ // If Log4j 1.2 is being used, but is not initialized, load a default properties file
+ val binderClass = StaticLoggerBinder.getSingleton.getLoggerFactoryClassStr
+ // This minimally distinguishes the log4j 1.2 binding, currently
+ // org.slf4j.impl.Log4jLoggerFactory, from the log4j 2.0 binding, currently
+ // org.apache.logging.slf4j.Log4jLoggerFactory
+ val usingLog4j12 =
+ binderClass.endsWith("Log4jLoggerFactory") && binderClass.startsWith("org.slf4j.")
+ val log4j12Initialized = LogManager.getRootLogger.getAllAppenders.hasMoreElements
+ if (!log4j12Initialized && usingLog4j12) {
val defaultLogProps = "org/apache/spark/log4j-defaults.properties"
Option(Utils.getSparkClassLoader.getResource(defaultLogProps)) match {
case Some(url) =>
PropertyConfigurator.configure(url)
- log.info(s"Using Spark's default log4j profile: $defaultLogProps")
--- End diff --
Sure - that's fine too. Basically, logging in general is a big confusing mystery to many beginner users, so I do want to have a message here that explains what is going on right up front.
I'd also be fine with just leaving the existing message with a comment that explains what is going on so that developers are guarded against changing it or trying to add further logging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50604660
Jenkins, retest this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-50448651
Thanks for catching this and digging into the fix. Some small questions in the PR, but generally looks good!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-2646. log4j initialization not quite com...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1547#issuecomment-49877595
QA tests have started for PR 1547. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17037/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---