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

Add a Jackson-based Java properties configuration factory #2170

Closed
wants to merge 8 commits into from

Conversation

ppkarwasz
Copy link
Contributor

This PR introduces a new JavaPropsConfigurationFactory, based on jackson-dataformat-properties.

In the future this factory might replace the old PropertiesConfigurationFactory based on the Builder API.

Pros

  • the mapping between properties and JSON is standardized by Jackson (cf. documentation) and we can refer users to their (somehow lacking) documentation.

Cons

  • this configuration factory does not support any of the quirks manually coded into the PropertiesConfigurationFactory, e.g.:
rootLogger = INFO, Console

users need to use the usual way to configure loggers or the undocumented levelAndRefs attribute:

Loggers.Root.levelAndRefs = INFO, Console

@jvz
Copy link
Member

jvz commented Jan 8, 2024

That workaround is a critical component for getting log4j v1 users to easily upgrade their configurations though.

@ppkarwasz
Copy link
Contributor Author

I believe you are referring to this dev@logging discussion.

If I understand correctly, Loggers.Root.levelAndRefs would be Ok for Hadoop users.

@vy
Copy link
Member

vy commented Jan 8, 2024

@ppkarwasz, I am puzzled. I was expecting to see some code removed too. Are we adding a 2nd .properties-formatted configuration factory next the what we already have?

That workaround is a critical component for getting log4j v1 users to easily upgrade their configurations though.

@jvz, even though you are right in theory, I would say it was more of a convenience for HBase, where they, in the past, made this convention get deployed during installation, and now they don't have control over those user-crafted files anymore, and hence they strive to support it. (LOG4J2-3341 contains the details.) I recall no other either a complaint for the absence of this feature, or a praise for the presence of it. Though all these don't make your remark less valid. We can indeed try to support it in log4j-config-properties. But... maybe... it is time for Log4j 1 users to put some effort into upgrading their configuration files?

@ppkarwasz
Copy link
Contributor Author

@vy,

This is more of a PoC. If you like it, I can remove/move PropertiesConfiguration and the Builder API in another PR.

@jvz
Copy link
Member

jvz commented Jan 8, 2024

I suppose it's not a blocker.

@vy
Copy link
Member

vy commented Jan 9, 2024

This is more of a PoC. If you like it, I can remove/move PropertiesConfiguration and the Builder API in another PR.

I think the Builder API and .properties-formatted configuration are two separate things. I really like the current PR, except it hosts two competing features. If you can remove the PropertiesConfiguration in log4j-core, I think we are done here. (We can address the Builder API discussion elsewhere.)

@ppkarwasz
Copy link
Contributor Author

@vy,

It is done, now we have two properties configuration factory modules:

  • log4j-config-properties-legacy contains the old code with a lot of exceptions to the mapping rules,
  • log4j-config-properties is Jackson-based and has simple rules to map from and to JSON.

For dropping log4j-config-properties-legacy, I believe we need a new PR.

Remark: I removed the if statement in LoggerConfig that prevented other configuration factories from using the levelAndRefs attribute.

The `Properties` node must traditionally be the first node in the
configuration. In order to support formats without a predefined order,
we remove this condition and do a search for a `Properties` node
instead.
The old properties configuration prevents the new one from having the
same configuration factory order.

This commit moves it to a new `log4j-config-properties-legacy` module.
@vy vy self-requested a review January 9, 2024 11:22
ppkarwasz and others added 2 commits January 9, 2024 12:34
Co-authored-by: Volkan Yazıcı <volkan@yazi.ci>
@ppkarwasz ppkarwasz self-assigned this Jan 9, 2024
@ppkarwasz ppkarwasz closed this Jan 9, 2024
@ppkarwasz ppkarwasz deleted the config-properties branch January 9, 2024 12:03
@ppkarwasz
Copy link
Contributor Author

This was actually merged in ec93d18

I deleted the upstream branch too fast (again) for Github to detect the merge.

@ppkarwasz ppkarwasz added this to the 3.0.0 milestone Jan 9, 2024
@ppkarwasz ppkarwasz modified the milestones: 3.0.0, 3.0.0-beta2 Feb 17, 2024
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.

3 participants