Skip to content

Remove Supplier #2175

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

Closed
wants to merge 2 commits into from
Closed

Remove Supplier #2175

wants to merge 2 commits into from

Conversation

vy
Copy link
Member

@vy vy commented Jan 10, 2024

Removes deprecated org.apache.logging.log4j.util.Supplier in favor of java.util.function.Supplier.

@vy vy added the api Affects the public API label Jan 10, 2024
@vy vy added this to the 3.0.0 milestone Jan 10, 2024
@vy vy self-assigned this Jan 10, 2024
@ppkarwasz
Copy link
Contributor

We discussed this many times and we can not do it, because of binary compatibility.

Users of log4j-api 3.x will still use libraries compiled against log4j-api 2.x.

What we can do is to create a v3.Logger interface like:

public interface Logger {
    public void debug(java.util.function.Supplier<?>);
    ...
}

and let the old interface extend it:

public interface Logger extends org.apache.logging.log4j.v3.Logger {
    public void debug(org.apache.logging.log4j.util.Supplier<?>);
    ...
}

@vy
Copy link
Member Author

vy commented Jan 10, 2024

log4j-api is already broken; log4j2-logstash-layout doesn't compile against version 3.0.0-beta1, you've just approved #2171, etc. Are you sure Supplier must stay?

@ppkarwasz
Copy link
Contributor

ppkarwasz commented Jan 10, 2024

Yes, I am sure. This was discussed in at least two meetings. I don't know if you were present, but @rgoers and @garydgregory can probably confirm.

We need to categorize the breaking changes into at least 3 categories:

  1. those that break most users code/binaries and we MUST NOT introduce them,
  2. those that break some users code/binaries and we SHALL NOT introduce them,
  3. those that only break the code of Log4j component providers and we MAY introduce them.

The Supplier class is referenced in the signature of Logger methods and is used by early adopters that didn't want to wait ~8 years for SLF4J to offer the same functionality. Your proposed change is source compatible, but it is not binary compatible, honestly I could not accept it.

I would categorize the changes in #2171 as category 2: I strongly believe that not many users reference ParameterizedMessageFactory in their code (even if MessageFactory is in signatures of LogManager methods). Same goes for Logger#entry and Logger#exit.

Regarding log4j2-logstash-layout, you used a class that was in a private package (o.a.l.l.util), so you were aware of the consequences. ;-) Only Spring has the power to keep using the internal PropertiesUtil class.

Remark: Of course I know that Supplier is also in o.a.l.l.util. When you look closely at the way packages interact, you understand that the subdivision into packages is chaotic and every breaking change in log4j-api requires an exhausting analysis.

Since SLF4J also has these API leakage properties (e.g. Logger references LoggingEventBuilder that is not in the API, but in the SPI package), can we move this PR to 4.x, when we'll have all the time to discuss every API break?

@rgoers
Copy link
Member

rgoers commented Jan 10, 2024

I would classify the breakage a little differently. We can expect that applications will upgrade the log4j version but still have dependencies on libraries compiled against the log4j 2 api. Those applications MUST continue to work. I am less concerned about minor changes in core that are resolved by a simple recompile as MOST applications will be recompiling to pick up Log4j 3.x anyway.
If we can use the interface approach to resolve migrate to j.u.f.Supplier I would be fine with that.

But as it stands this PR cannot be merged.

@vy
Copy link
Member Author

vy commented Jan 10, 2024

Our API is bloated, I presume this is public knowledge. I don't think we should be extending it with yet another Logger. I will close this PR. We can reconsider it in Log4j 9.

@vy vy closed this Jan 10, 2024
@vy vy deleted the main-remove-supplier branch January 10, 2024 15:22
@garydgregory
Copy link
Member

As I've mentioned before elsewhere I think we are taking the wrong path with version 3, we should be allowed to break BC in a major version. Then I'd expect to change the package name and maven coordinates to match. It's the only sane way to clean up our legacy public and protected APIs.

@jvz
Copy link
Member

jvz commented Jan 25, 2024

Any parallel v3 API will still require the Core to support both v2 and v3 APIs. Otherwise, you'd end up with at least two logging backends at the same time, and that's unlikely to be the desired behavior of anyone.

@ppkarwasz ppkarwasz removed this from the 3.0.0 milestone Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants