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

Liquibase 4.x Compatibility #503

Closed
wants to merge 2 commits into from

Conversation

nvoxland
Copy link

@nvoxland nvoxland commented Jun 2, 2021

Overview

Liquibase 3.6 as well as 4.0 introduced API compatibility issues with the current log4j-liquibase codebase. See liquibase/liquibase#1777

This updates the code to be compatible with 4.0+ while still supporting 3.5 and 3.6+ users.

@nvoxland
Copy link
Author

nvoxland commented Jun 2, 2021

With the new Log4j2Logger implementation, the messages coming through in the test class look like liquibase.ext.logging.log4j2.Log4j2LoggerTest liquibase.logging.core.AbstractLogger FATAL Severe message and I'm not sure where that "AbractLogger" part is coming from.

I think I'm setting up the ExtendedLogger the same as it was in the old code, but that extra piece is not showing up there.

Any ideas why?

@vy
Copy link
Member

vy commented Jun 8, 2021

Hey @nvoxland, thanks for the contribution. I have my concerns if Log4j should have a Liquibase-specific module in the first place. Let me elaborate on that a bit.

Certain libraries, which is Liquibase in this particular case, create their own logging API rather than choosing one available in the market, e.g., slf4j-api, log4j-api. Consequently, they contribute to logging backends (Logback, Log4j, etc.) with implementations of their custom logging API. This, IMHO, contradicts with the motives of those logging backends shipping a separate API in the first place. I would rather prefer Liquibase logging against, say, the Log4j API and let users pick a backend. For instance, users then can either employ any backend supporting Log4j API (e.g., Log4j itself) or use a bridge to port it to another API of their preference (e.g., using log4j-to-slf4j bridge in combination with Logback).

In your profile, I see that you are member of the Liquibase organization. Would you mind telling us why can't Liquibase simply use Log4j API instead? Are there any technical limitations I miss here?

@nvoxland
Copy link
Author

nvoxland commented Jun 9, 2021

From Liquibase's standpoint, as a library we try hard not to introduce dependencies on external libraries such as log4j. Therefore, we build off the java.util.Logger but do have a lightweight extension point where 3rd parties can plug in alternatives instead.

This code is really just plugging into that liquibase extension point to bypass JUL and go directly to log4j. An alternative is certainly to use the log4j-jul adapter as well. I didn't write this extension in the code here, just fixing it :)

If you are against continuing to have this extension as part of log4j, we could look at bringing it into the liquibase codebase directly as well. I don't think we could have it automatically enabled if log4j is found, since many users end up with log4j in their classpath even though it's not their "real" logging system. But, have some configuration settings we could support.

@vy
Copy link
Member

vy commented Jun 10, 2021

I perfectly understand your conservative attitude for adding dependencies to your project and share them. This said, dependencies increase re-usability and independent evolution. Hence, I strongly kindly advice you to consider adopting log4j-api, if possible.

Even though I am a PMC member, I cannot speak for the rest of the board. I will raise this issue in the dev mailing list. If we decide to exclude log4j-liquibase from 3.x onward, I see two possible paths:

  1. Liquibase adopting log4j-api (recommended)
  2. Introducing liquibase-log4j module (I already see a org.liquibase.ext:liquibase-javalogger module)

For the records, would you mind sharing this with the rest of Liquibase crew and let us know what do you think, please? Is adopting log4j-api really a no-go for you?

@nvoxland
Copy link
Author

Yes, I will keep the rest of the team up on this discussion.

I think that making log4j-api a required part of our API or standard logging implementation is a no-go. It would mean having to change a stable API, and impacts users embedding liquibase but not wanting to introduce log4j-api as a dependency into their software.

But, there are ways we can include it in liquibase so log4j isn't required but still would work when people already have it as a dependency.

@rgoers
Copy link
Member

rgoers commented Jun 10, 2021

I don't necessarily object to Log4j providing integrations with third party components. But if something like liquibase chooses to provide its own logging API then it should also supply the bindings to the various implementations its users want to use. That binding could certainly be hosted inside the Log4j project, but it really shouldn't be part of the main Log4j release. We can't possibly support every such integration in that way.

@camsaul
Copy link

camsaul commented Mar 11, 2022

@ppkarwasz
Copy link
Contributor

I close this, since Liquibase will publish their own plugin (cf. #1193).

@nvoxland, thank You for your contribution.

@ppkarwasz ppkarwasz closed this Jan 16, 2023
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.

5 participants