Skip to content

Conversation

@sarutak
Copy link
Member

@sarutak sarutak commented Dec 8, 2019

What changes were proposed in this pull request?

In the current implementation of SparkShellLoggingFilter, if the log level of the root logger and the log level of a message are different, whether a message should logged is decided based on log4j's configuration but whether the message should be output to the REPL's console is not cared.
So, if the log level of the root logger is DEBUG, the log level of REPL's logger is WARN and the log level of a message is INFO, the message will output to the REPL's console even though INFO < WARN.
https://github.com/apache/spark/pull/26798/files#diff-bfd5810d8aa78ad90150e806d830bb78L237

The ideal behavior should be like as follows and implemented them in this change.

  1. If the log level of a message is greater than or equal to the log level of the root logger, the message should be logged but whether the message is output to the REPL's console should be decided based on whether the log level of the message is greater than or equal to the log level of the REPL's logger.

  2. If a log level or custom appenders are explicitly defined for a category, whether a log message via the logger corresponding to the category is logged and output to the REPL's console should be decided baed on the log level of the category.
    We can confirm whether a log level or appenders are explicitly set to a logger for a category by Logger#getLevel and Logger#getAllAppenders.hasMoreElements.

Why are the changes needed?

This is a bug breaking a compatibility.

#9816 enabled REPL's log4j configuration to override root logger but #23675 seems to have broken the feature.
You can see one example when you modifies the default log4j configuration like as follows.

# Change the log level for rootCategory to DEBUG
log4j.rootCategory=DEBUG, console

...
# The log level for repl.Main remains WARN
log4j.logger.org.apache.spark.repl.Main=WARN

If you launch REPL with the configuration, INFO level logs appear even though the log level for REPL is WARN.

・・・

19/12/08 23:31:38 INFO Utils: Successfully started service 'sparkDriver' on port 33083.
19/12/08 23:31:38 INFO SparkEnv: Registering MapOutputTracker
19/12/08 23:31:38 INFO SparkEnv: Registering BlockManagerMaster
19/12/08 23:31:38 INFO BlockManagerMasterEndpoint: Using org.apache.spark.storage.DefaultTopologyMapper for getting topology information
19/12/08 23:31:38 INFO BlockManagerMasterEndpoint: BlockManagerMasterEndpoint up
19/12/08 23:31:38 INFO SparkEnv: Registering BlockManagerMasterHeartbeat

・・・

Before #23675 was applied, those INFO level logs are not shown with the same log4j.properties.

Does this PR introduce any user-facing change?

Yes. The logging behavior for REPL is fixed.

How was this patch tested?

Manual test and newly added unit test.

@SparkQA
Copy link

SparkQA commented Dec 8, 2019

Test build #114993 has finished for PR 26798 at commit 2aa61b1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member

cc @ankuriitg and @vanzin

val infoLogMessage2 = "infoLogMessage3 should be output"

val out = try {
PropertyConfigurator.configure(log4jprops.toString)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.getAbsolutePath()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I've replaced it with absolute path.

Make config-file-path-string absolute path
@sarutak sarutak force-pushed the fix-spark-shell-loglevel branch from 28531a3 to df78a35 Compare December 13, 2019 06:45
@SparkQA
Copy link

SparkQA commented Dec 13, 2019

Test build #115284 has finished for PR 26798 at commit df78a35.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sarutak
Copy link
Member Author

sarutak commented Dec 13, 2019

retest this please.

@SparkQA
Copy link

SparkQA commented Dec 13, 2019

Test build #115287 has finished for PR 26798 at commit df78a35.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin
Copy link
Contributor

vanzin commented Dec 13, 2019

Ok, I got what was wrong before (and with the previous unit test). Merging to master.

@vanzin vanzin closed this in 61ebc81 Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants