Skip to content

[MNG-7083] - introduce lazy Log message evaluation#438

Closed
McFoggy wants to merge 6 commits intoapache:masterfrom
McFoggy:MNG-7083
Closed

[MNG-7083] - introduce lazy Log message evaluation#438
McFoggy wants to merge 6 commits intoapache:masterfrom
McFoggy:MNG-7083

Conversation

@McFoggy
Copy link
Copy Markdown

@McFoggy McFoggy commented Jan 26, 2021

as discussed in the dev mailing list [1][2] this PR enhances org.apache.maven.plugin.logging.Log by adding a lazy log method with a java.util.function.Supplier for each log level.
For each level the supplier will be called only if the corresponding log level is active.
Using java 8 interface default method ensures backward compatibility with other potential existing implementations.

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MNG-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MNG-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the Core IT

Still two unrelatd ITs tests are failing locally

[ERROR] Errors:
[ERROR]   MavenITmng5669ReadPomsOnce>AbstractMavenIntegrationTestCase.runTest:255->testWithBuildConsumer:119 » Verification
[ERROR]   MavenITmng5669ReadPomsOnce>AbstractMavenIntegrationTestCase.runTest:255->testWithoutBuildConsumer:65 » Verification

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

PS: I'll verify by asking on the mailing list if I have an ICLA already, or fill one if not.

Enhance org.apache.maven.plugin.logging.Log by adding lazy java.util.function.Supplier for each log level.
For each level the supplier will be called only if the corresponding log level is active.
Using java 8 interface default method ensures backward compatibility with other potential implementations.
@McFoggy
Copy link
Copy Markdown
Author

McFoggy commented Jan 26, 2021

Initially I did not wanted to pollute stdout & stderr that's why I mocked them in SystemStreamLogTest.
Thinking more about it, it can be a bad idea to have mocked them.
If required I'll remove the mock and let the SystemStreamLogTest test output messages.

Comment thread maven-plugin-api/src/main/java/org/apache/maven/plugin/logging/Log.java Outdated
Copy link
Copy Markdown
Contributor

@rmannibucau rmannibucau left a comment

Choose a reason for hiding this comment

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

Two comments about the testing and default usage, otherwise looks good to me.

Comment thread maven-plugin-api/pom.xml Outdated
Comment thread maven-plugin-api/src/main/java/org/apache/maven/plugin/logging/Log.java Outdated
@michael-o
Copy link
Copy Markdown
Member

I really can't believe we are reinventing the wheel by implementing SLF4J Light.

@McFoggy
Copy link
Copy Markdown
Author

McFoggy commented Jan 26, 2021

I really can't believe we are reinventing the wheel by implementing SLF4J Light.

that's not what I understood from the different discussions.
I really thought that it was expected to leverage the org.apache.maven.plugin.logging.Log abstraction which represents the public interface that plugins should see and use.

@rmannibucau
Copy link
Copy Markdown
Contributor

@McFoggy it is the outcome of the discussions because slf4j is 1. unstable, 2. has no correct binding impl (it is a singleton leading to a bunch of issues in mojo when transparently exposed) 3. does not enable to switch the binding until you reimpl slf4j by design 4. is not contextual with most available impl and 5. we already reimplement slf4j and just inherit from a small API part but not the rest. I fully understand it can be surprising but it is the outcome of most vendors in most projects caring of the context and to have a pluggable logging impl so it is not that shocking after some thoughts as seen in the discussion.

@michael-o
Copy link
Copy Markdown
Member

@McFoggy it is the outcome of the discussions because slf4j is 1. unstable, 2. has no correct binding impl (it is a singleton leading to a bunch of issues in mojo when transparently exposed) 3. does not enable to switch the binding until you reimpl slf4j by design 4. is not contextual with most available impl and 5. we already reimplement slf4j and just inherit from a small API part but not the rest. I fully understand it can be surprising but it is the outcome of most vendors in most projects caring of the context and to have a pluggable logging impl so it is not that shocking after some thoughts as seen in the discussion.

I would expect such discussions to be held with @ceki instead with just us.

…face

introduction of an AbstractLog clas sto share default methods implementations
@McFoggy
Copy link
Copy Markdown
Author

McFoggy commented Jan 26, 2021

FYI ICLA received & handled by Apache secretary, signed CLAs list updated (https://people.apache.org/unlistedclas.html)

@McFoggy
Copy link
Copy Markdown
Author

McFoggy commented Jan 26, 2021

If asked for, I will squash the commits.

@slachiewicz
Copy link
Copy Markdown
Member

I'd like to see how much we can improve our core code with these changes

@cstamas
Copy link
Copy Markdown
Member

cstamas commented Oct 2, 2021

I really don't think we want to invent new logging API. Let me explain:

  • I do agree with @rmannibucau et al that "slf4j-api should not leak into plugins", but IMHO none of the reasons listed above is the real issue.
  • IMHO, the real issue is that slf4j-api is "like Guava", is literally everywhere, and (in short term future) will be really easy to end up (in complex plugins, kinda edge case) with a class-path that contains 1.5, 1.7 and 2.x versions of slf4j-api (and let's assume they will be incompatible).
  • This is true for complex plugins that usually drag in components from different dependencies/projects. OTOH, simple plugins (dependency wise, not functionality or logic wise) that has only components "rolled by it's own" CAN control which slf4j-api Logging API it expects and uses.
  • Hence, IMHO Maven should not be an impediment (or trouble source) to developers of "complex plugins", by adding a "yet another" slf4j-api version to their (possibly already complex) equation to solve.

Still, as plugin realms are "self first", I think plugin developers are able to solve Mojo (and their component, rolled by own or imported from foreign projects a dependencies) using any logging API they want.

Considering all this above, I'd rather expect to make Mojo Logger "closer" to slf4j-api, not only to simplify our work (to adapt slf4j logger to Mojo Logger), but also as sl4j logger is lazy as well, without any extra code. As in case of Mojo Logger, our wrapper will protect users of Mojo logger from sl4j-api incompatibilities (is provided at runtime by Maven Core).

@cstamas
Copy link
Copy Markdown
Member

cstamas commented Oct 5, 2021

Proposal to supersede this #565

@McFoggy McFoggy mentioned this pull request Oct 11, 2021
@slawekjaranowski slawekjaranowski added the waiting-for-feedback Waiting for 90 days until issues or pull request will be closed label Feb 15, 2025
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2025

This pull request is stale because it has been waiting for feedback for 60 days. Remove the stale label or comment on this PR, or it will be automatically closed in 30 days.

@github-actions github-actions Bot added the Stale label May 5, 2025
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 4, 2025

This pull request has been closed because no response was received within 90 days.

@github-actions github-actions Bot closed this Jun 4, 2025
@jira-importer
Copy link
Copy Markdown

Resolve #8370

@github-actions github-actions Bot removed waiting-for-feedback Waiting for 90 days until issues or pull request will be closed Stale labels Jul 6, 2025
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.

7 participants