Skip to content

Use deprecation logger holder in byte size value #53928

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

Conversation

jasontedor
Copy link
Member

If a setting is touched during bootstrap before logging is configured, and that setting uses a byte size value, the deprecation logger for ByteSizeValue will be initialized. However, this means a logger will be configured before log4j is initialized, which we reject at startup. This commit puts this deprecation logger in a holder pattern so that it is not initialized until first use, which will happen after logging is configured.

If a setting is touched during bootstrap before logging is configured,
and that setting uses a byte size value, the deprecation logger for
ByteSizeValue will be initialized. However, this means a logger will be
configured before log4j is initialized, which we reject at startup. This
commit puts this deprecation logger in a holder pattern so that it is
not initialized until first use, which will happen after logging is
configured.
@jasontedor jasontedor added >non-issue :Core/Infra/Core Core issues without another label v7.7.0 labels Mar 21, 2020
@rjernst
Copy link
Member

rjernst commented Mar 22, 2020

Could we do a similar thing, but within DeprecationLogger's use of Logger, so that we don't accidentally do this again? The new pattern here of using DeprecationLogger lazily isn't documented or enforced, so I'm afraid we will have more similar cases with no way to detect until we see it blow up.

@jasontedor
Copy link
Member Author

Could we do a similar thing, but within DeprecationLogger's use of Logger, so that we don't accidentally do this again?

@rjernst The build will not even start if we do this wrongly, we already have detection for that. The reason I am doing this here is because I need it for another pull request which exposed this logger to startup, when it wasn't exposed before. See #53924.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@jasontedor jasontedor merged commit bc7b995 into elastic:7.x Mar 23, 2020
jasontedor added a commit that referenced this pull request Mar 23, 2020
If a setting is touched during bootstrap before logging is configured,
and that setting uses a byte size value, the deprecation logger for
ByteSizeValue will be initialized. However, this means a logger will be
configured before log4j is initialized, which we reject at startup. This
commit puts this deprecation logger in a holder pattern so that it is
not initialized until first use, which will happen after logging is
configured.
@jasontedor jasontedor deleted the use-deprecation-logger-holder-in-byte-size-value branch March 23, 2020 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants