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.

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

[Issue2281] The core.Logger#setLevel method should work like Configurator#setLevel() #2289

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

garydgregory
Copy link
Member

Do for this module what we did for log4j-1.2-api.

@garydgregory
Copy link
Member Author

Note that the new test fails... @ppkarwasz

@ppkarwasz
Copy link
Contributor

Are we testing the right thing? Unless I am mistaken JUL's Logger#getLevel returns the explicitly configured level of the logger. A call to Logger#setLevel should not change the explicit level of any other logger, but it should change the effective level of all its children.

Unfortunately the effective level can be only be guessed by calling Logger#isLoggable multiple times.

I think that our CoreLogger implementation has an additional bug: it does not overwrite Logger#getLevel. So if a user changes the level of a logger through Log4j Core directly, CoreLogger will not be able to detect it.

@garydgregory
Copy link
Member Author

garydgregory commented Feb 15, 2024

Ah, good point on the tests, I updated the tests to use isLoggable() and now they pass.

Configurator#setLevel apache#2281

Do for this module what we did for log4j-1.2-api
Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

LGTM

@garydgregory garydgregory merged commit 9e71d44 into apache:2.x Feb 15, 2024
9 checks passed
@garydgregory garydgregory deleted the 2.x-jul-issue-2281 branch February 15, 2024 21:08
@garydgregory
Copy link
Member Author

LGTM

@ppkarwasz
TY for your help; merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants