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

Package naming conventions #56

Open
globalbus opened this issue Oct 29, 2020 · 11 comments
Open

Package naming conventions #56

globalbus opened this issue Oct 29, 2020 · 11 comments

Comments

@globalbus
Copy link

I'm using this brilliant library in modular project (OSGi).
Of course library is not OSGi complaint, but bunch of wrapping instructions and dirty solution for refreshing log4j2 PluginRegistry and log4j2 context is enough to make it working in apache karaf environment.

Newer versions are much more problematic with running in modular environment. It's because weird package naming inside jars.
In example, inside log4j2-elasticsearch-core we have

  • com.fasterxml.jackson.core
  • org.apache.logging.log4j.core.jackson
  • org.appenders.core.logging
  • org.appenders.log4j2.elasticsearch

For now, biggest fault is com.fasterxml.jackson.core. That package is already present in jackson-core and that thing breaks package resolving.

There is a special reason for that kind of packaging? Most of projects in java world tries to keep things under single package root, equal to project groupId + moduleName. That really helps with modularity problems.

@rfoltyns
Copy link
Owner

Thank you for the good word! 👍

I agree, unfortunately basic good practices are not followed when it comes to package names. In most cases it's due to the need to access package-private or protected classes, either for extensions (see *MixIn classes) or to introduce some performance improvements - com.fasterxml.jackson.core is a good example as it needs to do some wizardry with JsonWriteContext in single threaded mode.

Also, most classes are just dumped into the main package - this will change in 2.0 (end of next year at least) when core will be split into multiple modules.

I'll have a look at those packages and see what I can do. So far:

  • com.fasterxml.jackson.core package along with the rest of single thread optimisations are good candidates for extraction to separate jar (it would happen eventually anyway)
  • org.appenders.core.logging is one of the first steps towards repackaging in 2.0 - may as well be a separate jar already (it would happen anyway)
  • org.apache.logging.log4j.core.jackson needs access to Log4j2 mixins - otherwise I'd have to copy those classes 1-to-1.. maybe a thirdparty package would be a good one for these ones?

Is this issue blocking you completely atm? Is 1.4.4 unusable for you?

@globalbus
Copy link
Author

I'm not blocked, that's a kind of general question. I'm doing modules since years, but my descendants in company could be not very happy after taking over such legacy.

If I simply say "I will not use singleThread option", package 'com.fasterxml.jackson.core' is optional. I must omit this export and that's all (classes from this package will be never loaded, if this option is disabled).
Splitting that to separate jars will be a good approach. Then I could load 'com.fasterxml.jackson.core' as fragment bundle (kind of trick to attach it to jackson-core bundle). But keeping all of thirdparties in single jar will be also problematic (I can attach fragment to only one host bundle).

@rfoltyns
Copy link
Owner

"I will not use singleThread option", package 'com.fasterxml.jackson.core' is optional. I must omit this export and that's all (classes from this package will be never loaded, if this option is disabled)

I was really hoping that this single design decision and classloading tricks will do the job for all cases..

Separate jar for jackson package-private classes would also contain actual SingleThreadJsonFactory from org.appenders.log4j2.elasticsearch. I'm not sure how well fragment bundles would handle that.

Shading classes into org.appenders.log4j2.elasticsearch.thirdparty might be a way to go for Log4j2 classes here. Similar approach was taken in Netty project - JCTools collections are shaded to prevent any classloading issues. I was reluctant to do it this way, but I might have to re-consider.

@globalbus
Copy link
Author

globalbus commented Oct 29, 2020

will do the job for all cases..

For openjdk, I would say - yes. I'm only using this for container environment in kubernetes, so I could feel safe with that (no other implementation could be used accidentally).

Separate jar for jackson package-private classes would also contain actual SingleThreadJsonFactory from org.appenders.log4j2.elasticsearch. I'm not sure how well fragment bundles would handle that.

It would handle, but if SingleThreadJsonFactory have their separate unique package name (that could be exported and consumed elsewhere).

Yes, I'm also not a huge fan of shading classes. But decision about that should be consulted with question "Am I using internal APIs of third party library, that could be incompatibly changed between versions without notice?". Jackson library in the past had breaking changes between minor versions.
Anyway, sometimes it's easier to handle such things with reflection (checking unaccessible field existence before reading). And reflection it's pretty fast in last JRE implementations. Or even multi approach, checking conditions via reflection before loading faster hacks.

@rfoltyns
Copy link
Owner

Incompatibility is a risk with both reflection and package-private access approaches. I'll be very reluctant to add any reflection-based logic to the code base. If there's a need for a package-private access, I'd rather try to contribute to the source project to make this access easier.

reflection it's pretty fast in last JRE implementations

Regardless of latency, it just feels wrong. If property is private, there's a reason for it. If it's package-private or protected - I can take the risk, especially if it's on the hot path.

@rfoltyns
Copy link
Owner

Thankfully, repackaging will be possible after jackson-core:2.12.0 release. Thanks, Tatu! 👍

I'll release it in 1.5 if possible.

@globalbus
Copy link
Author

Good to hear. Please, send me a notification before release, I will try to check it for compatibility in modular system.

rfoltyns added a commit that referenced this issue Dec 11, 2020
rfoltyns added a commit that referenced this issue Feb 10, 2021
…on-st:1.0 (#56)

* In order to avoid package-related issues in OSGi environments, custom Jackson FasterXML JsonFactory was extracted to separate module
@rfoltyns
Copy link
Owner

com.fasterxml.jackson.core classes were moved to appenders-jackson-st. compile scope, but not required by default.

I'll extract org.appenders.core.logging in 1.5.0 as well.

Can you handle org.apache.logging.log4j.core.jackson? I'd would just deprecate it and repackage in 2.0.

@globalbus
Copy link
Author

looks good for me. I really appreciate it.

with org.apache.logging.log4j.core.jackson I have no problems at the moment, because it's not exported explicitly by pax-logging (no version conflict).

@rfoltyns
Copy link
Owner

org.appenders.core.logging classes were moved to appenders-logging.

I pushed latest log4j2-elasticsearch snapshots to Sonatype repos.

@rfoltyns
Copy link
Owner

Released in 1.5.0.

org.apache.logging.log4j.core.jackson will be moved in 2.0.

Enjoy!

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

No branches or pull requests

2 participants