Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "Set default root log level to debug" and make PULSAR_LOG_ROOT_LEVEL to default to PULSAR_LOG_LEVEL #12941

Merged
merged 2 commits into from
Nov 25, 2021
Merged

Revert "Set default root log level to debug" and make PULSAR_LOG_ROOT_LEVEL to default to PULSAR_LOG_LEVEL #12941

merged 2 commits into from
Nov 25, 2021

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Nov 23, 2021

Motivation

Reverts #7789 which had this motivation:

Since currently the root log level is set to info, it is not easy to turn the logging level
to debug by running PULSAR_LOG_LEVEL=debug bin/pulsar standalone.

This PR reverts the change to set root log level to debug since it can cause many performance issues (such as #8908) when Logger.isDebugEnabled() defaults to true.
This PR provides an alternative solution to enable debug logging. The expectation is that debug logging could be enabled by specifying PULSAR_LOG_LEVEL=debug. The solution is to make PULSAR_LOG_ROOT_LEVEL default to PULSAR_LOG_LEVEL.

Modifications

Documentation

  • [x ] no-need-doc

@lhotari lhotari added type/performance type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use labels Nov 23, 2021
@lhotari lhotari added this to the 2.10.0 milestone Nov 23, 2021
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 23, 2021
eolivelli
eolivelli approved these changes Nov 23, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

@codelipenghui codelipenghui merged commit 959430c into apache:master Nov 25, 2021
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
@lhotari @eolivelli
Revert "Set default root log level to debug" and make PULSAR_LOG_ROOT…
9e8b21e 
…_LEVEL to default to PULSAR_LOG_LEVEL (apache#12941)
lhotari added a commit that referenced this pull request Dec 9, 2021
@lhotari
Revert "Set default root log level to debug" and make PULSAR_LOG_ROOT…
eca18ca 
…_LEVEL to default to PULSAR_LOG_LEVEL (#12941)

(cherry picked from commit 959430c)
lhotari added a commit that referenced this pull request Dec 9, 2021
@lhotari
Revert "Set default root log level to debug" and make PULSAR_LOG_ROOT…
137913f 
…_LEVEL to default to PULSAR_LOG_LEVEL (#12941)

(cherry picked from commit 959430c)
@lhotari lhotari added cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life labels Dec 9, 2021
lhotari added a commit to datastax/pulsar that referenced this pull request Dec 10, 2021
@lhotari
Revert "Set default root log level to debug" and make PULSAR_LOG_ROOT…
1f43bcc 
…_LEVEL to default to PULSAR_LOG_LEVEL (apache#12941)

(cherry picked from commit 959430c)
(cherry picked from commit eca18ca)
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
@lhotari
Revert "Set default root log level to debug" and make PULSAR_LOG_ROOT…
2ab717b 
…_LEVEL to default to PULSAR_LOG_LEVEL (apache#12941)
@lordcheng10
Copy link
Contributor

After this change, can we still dynamically adjust the log level to debug during runtime? @lhotari

@lhotari
Copy link
Member Author

lhotari commented Jan 28, 2022

After this change, can we still dynamically adjust the log level to debug during runtime? @lhotari

Please provide the detailed steps to reproduce. The PR doesn't prevent anything and setting could be set with environment variables. Since things have changed you might have to change your approach. Please reply to the issue or file a new issue so that the information gets shared with others.

@lhotari lhotari deleted the lh-revert-default-root-log-level branch January 28, 2022 06:26
@lhotari
Copy link
Member Author

lhotari commented Jan 28, 2022

@lordcheng10 How do you dynamically adjust the log level? Explaining that could help understanding what the problem is.
some context why the root log level shouldn't be needlessly at debug: #8908 .

since the root log level was debug, all code blocks within
log.isDebugEnabled() got executed.
This caused a lot of unnecessary memory allocations and wasted CPU cycles.

The dynamic configuration support should support also adjusting the root logging level.

@nodece nodece mentioned this pull request Feb 3, 2024
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@eolivelli eolivelli eolivelli approved these changes

@ivankelly ivankelly Awaiting requested review from ivankelly

@merlimat merlimat Awaiting requested review from merlimat

@sijie sijie Awaiting requested review from sijie

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

@aahmed-se aahmed-se Awaiting requested review from aahmed-se

Assignees

@lhotari lhotari

Labels
cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.2 release/2.9.1 type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@lhotari @lordcheng10 @eolivelli @codelipenghui @michaeljmarshall