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

DynLogEnvironment - Also wrap configuration with a LazyInitializer. #182

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

j-denner
Copy link
Contributor

@j-denner j-denner commented Jul 3, 2024

DynLogEnvironment - Also wrap configuration with a LazyInitializer.

In the default flow, the wrapping would not be necessary. But since the DynamicLogLevelConfiguration is also exposed via protected Optional<DynamicLogLevelConfiguration> getConfiguration(), it is recreated on each invocation.
(Another solution would be to remove getConfiguration(), but this is breaking API contracts)

* In the default flow it is not necessary. But since the DynamicLogLevelConfiguration is also exposed via the protected getConfiguration(), it is recreated on each invocation.
Copy link
Contributor

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

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

Thanks for providing this change. This seems to be rather hard to notice. Thanks for doing so. Note, that this code will change in 4.x, where the configuration will be imported via SPI. For now, this is not yet implemented.

@KarstenSchnitter
Copy link
Contributor

@j-denner, how urgently do you need this change? I would want to include #181 and this for the next release.

@j-denner
Copy link
Contributor Author

j-denner commented Jul 5, 2024

@KarstenSchnitter Whenever it fits best for you. I do not depend on this fix and provided a PR to point out/correct it.
(I found it while working on a jax rs integration for logging)

@KarstenSchnitter KarstenSchnitter merged commit 437073e into SAP:main Jul 9, 2024
2 checks passed
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