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

Move Recycler to log4j-kit #2400

Merged
merged 3 commits into from
Mar 28, 2024
Merged

Conversation

ppkarwasz
Copy link
Contributor

This PR depends on #2396.

We move Recycler interface introduced in #1969 from log4j-api to log4j-kit.

Since the only current user of Recycler is Log4j Core 3.x, we removed the static recycler registry and replaced it with an entry in DefaultBundle.

This also closes #2106.

Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

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

I completely agree that recycler concept should not be part of the -api, but -kit. Hence, great initiative! 💯

I don't see any blockers, though dropped some minor remarks/questions.

I have the impression that certain file move+edit ops could not get tracked by Git properly and hence appear as new files. This makes reviewing difficult. I don't know if there is much you can do about it. But wanted to share it anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this class be located in a log4j-kit-test artifact instead? I know, this implies moving log4j-kit/src/test to log4j-kit-test/src/test, but be it, just like log4j-core-test and log4j-api-test.

Could you also add some Javadoc, please?

@vy
Copy link
Member

vy commented Mar 25, 2024

Since the only current user of Recycler is Log4j Core 3.x, we removed the static recycler registry and replaced it with an entry in DefaultBundle.

This is not correct. There are other modules besides log4j-core that use recyclers, for instance JTL.

@ppkarwasz
Copy link
Contributor Author

Since the only current user of Recycler is Log4j Core 3.x, we removed the static recycler registry and replaced it with an entry in DefaultBundle.

This is not correct. There are other modules besides log4j-core that use recyclers, for instance JTL.

What I meant is that only Log4j Core and its sub-modules use recyclers explicitly. Implementations such as log4j-to-jul and log4j-to-slf4j are based purely on log4j-api, so they will loose Recycler support after Log4j API 3.x is removed.

Base automatically changed from fix/apply-property-environment to feature/log4j-sdk March 28, 2024 10:27
@ppkarwasz ppkarwasz merged commit 7624f54 into feature/log4j-sdk Mar 28, 2024
5 checks passed
@ppkarwasz ppkarwasz deleted the fix/move-recycler-to-kit branch March 28, 2024 10:27
@ppkarwasz ppkarwasz mentioned this pull request Mar 28, 2024
8 tasks
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