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

[improve][misc] Upgrade slf4j to 2.0.13 #22391

Merged
merged 4 commits into from
Apr 25, 2024

Conversation

nodece
Copy link
Member

@nodece nodece commented Apr 1, 2024

Motivation

Keep up-to-date, and this version supports fluent logging api.

Modifications

  • Upgrade slf4j from 1.7.32 to 2.0.13
  • Upgrade qpid-server to adapt the slf4j 2
  • Use log4j-slf4j2-impl instead log4j-slf4j-impl
  • Exclude the log4j-slf4j-impl and slf4j from some library

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@nodece nodece self-assigned this Apr 1, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 1, 2024
@nodece nodece force-pushed the upgrade-slf4j-2.0.12 branch 4 times, most recently from 7aef84a to de9c7ad Compare April 2, 2024 03:57
@lhotari
Copy link
Member

lhotari commented Apr 2, 2024

@nodece license check fails

@nodece nodece force-pushed the upgrade-slf4j-2.0.12 branch 6 times, most recently from 88da8ca to a942ff1 Compare April 3, 2024 17:34
@nodece
Copy link
Member Author

nodece commented Apr 4, 2024

I downloaded the test image and then tried to test the CLI test, when proxy is start, Failed to load class "org.slf4j.impl.StaticLoggerBinder" doesn't be printed, but in the GHA, this message has been printed, please see https://productionresultssa13.blob.core.windows.net/actions-results/c3508721-b89b-426a-aa66-cd2a8c2f5a99/workflow-job-run-6b1ea721-31e7-5262-bdcc-795f87829cb5/logs/job/job-logs.txt?rsct=text%2Fplain&se=2024-04-04T17%3A59%3A14Z&sig=lMZ7Bk7gThEIkAHVFAbJLNchc2Nq2jgfkcFXlxcWf%2F8%3D&sp=r&spr=https&sr=b&st=2024-04-04T17%3A49%3A09Z&sv=2021-12-02

unzip -p pulsar-java-test-image.zst.zip | unzstd | docker load
cd $PULSAR_HOME
./build/run_integration_group.sh CLI

@lhotari Do you have any suggestions?

@lhotari
Copy link
Member

lhotari commented Apr 5, 2024

@lhotari Do you have any suggestions?

@nodece To debug, locally with java-test-image in addition what you provided, you'd have to do export PULSAR_TEST_IMAGE_NAME=apachepulsar/java-test-image:latest (ref:

PULSAR_TEST_IMAGE_NAME: apachepulsar/java-test-image:latest
) before running the tests locally. The default value for PULSAR_TEST_IMAGE_NAME is the pulsar-test-latest-version image (ref:
public static final String DEFAULT_IMAGE_NAME = System.getenv().getOrDefault("PULSAR_TEST_IMAGE_NAME",
"apachepulsar/pulsar-test-latest-version:latest");
).

@nodece
Copy link
Member Author

nodece commented Apr 5, 2024

Sorry, in my test, PULSAR_TEST_IMAGE_NAME has been set, but it still does not work.

@lhotari
Copy link
Member

lhotari commented Apr 5, 2024

Sorry, in my test, PULSAR_TEST_IMAGE_NAME has been set, but it still does not work.

@nodece What doesn't work?

@nodece
Copy link
Member Author

nodece commented Apr 6, 2024

Sorry, in my test, PULSAR_TEST_IMAGE_NAME has been set, but it still does not work.

@nodece What doesn't work?

In the GHA, the CLI test always print the “Failed to load class "org.slf4j.impl.StaticLoggerBinder"”, but in my local, never print.

This info means we are using the slf4j 1.x, not slf4j 2.x.

GHA:

2024-04-05T09:39:07.1983878Z 2024-04-05T09:39:07,145 - INFO  - [main:PulsarContainer@253] - [ClientToolTest-myqpv-pulsar-proxy] Start pulsar service pulsar-proxy at container d1c4d0f4a3dfb6d91c7f230a21987710ee009db3d3646e745cf07164a2f75e9d
2024-04-05T09:39:07.1986638Z 2024-04-05T09:39:07,146 - INFO  - [main:PulsarCluster@364] - Successfully started pulsar proxy.
2024-04-05T09:39:07.1988917Z 2024-04-05T09:39:07,146 - INFO  - [main:PulsarCluster@366] - Pulsar cluster ClientToolTest-myqpv is up running:
2024-04-05T09:39:07.2099421Z 2024-04-05T09:39:07,146 - INFO  - [main:PulsarCluster@367] - 	Binary Service Url : pulsar://localhost:32778
2024-04-05T09:39:07.2101524Z 2024-04-05T09:39:07,147 - INFO  - [main:PulsarCluster@368] - 	Http Service Url : http://localhost:32777
2024-04-05T09:39:07.2103743Z 2024-04-05T09:39:07,181 - INFO  - [docker-java-stream--13958939:DockerUtils$4@378] - DOCKER.exec(ClientToolTest-myqpv-pulsar-proxy:tail -f /var/log/pulsar/proxy.log): Executing...
2024-04-05T09:39:07.2906604Z 2024-04-05T09:39:07,259 - INFO  - [docker-java-stream--13958939:DockerUtils$4@383] - DOCKER.exec(ClientToolTest-myqpv-pulsar-proxy:tail -f /var/log/pulsar/proxy.log): STDOUT: SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
2024-04-05T09:39:07.2909433Z SLF4J: Defaulting to no-operation (NOP) logger implementation
2024-04-05T09:39:07.2910970Z SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
2024-04-05T09:39:07.2912410Z SLF4J: Failed to load class "org.slf4j.impl.StaticMDCBinder".
2024-04-05T09:39:07.2913850Z SLF4J: Defaulting to no-operation MDCAdapter implementation.
2024-04-05T09:39:07.2915287Z SLF4J: See http://www.slf4j.org/codes.html#no_static_mdc_binder for further details.

Local:

2024-04-07T11:27:48,514 - INFO  - [main:PulsarContainer@253] - [ClientToolTest-nwtuv-pulsar-proxy] Start pulsar service pulsar-proxy at container 789fdee1017c69b922f18ad8bea2622a54cb4267edbafb172da570a730a40e98
2024-04-07T11:27:48,515 - INFO  - [main:PulsarCluster@364] - Successfully started pulsar proxy.
2024-04-07T11:27:48,515 - INFO  - [main:PulsarCluster@366] - Pulsar cluster ClientToolTest-nwtuv is up running:
2024-04-07T11:27:48,520 - INFO  - [main:PulsarCluster@367] -    Binary Service Url : pulsar://localhost:55328
2024-04-07T11:27:48,520 - INFO  - [docker-java-stream-1508499096:DockerUtils$4@378] - DOCKER.exec(ClientToolTest-nwtuv-pulsar-proxy:tail -f /var/log/pulsar/proxy.log): Executing...
2024-04-07T11:27:48,521 - INFO  - [main:PulsarCluster@368] -    Http Service Url : http://localhost:55329
2024-04-07T11:27:48,602 - INFO  - [docker-java-stream-1508499096:DockerUtils$4@383] - DOCKER.exec(ClientToolTest-nwtuv-pulsar-proxy:tail -f /var/log/pulsar/proxy.log): STDOUT: 2024-04-07T03:27:48,258+0000 [main] INFO  org.eclipse.jetty.server.handler.ContextHandler - Started o.e.j.s.ServletContextHandler@c7f4457{/,null,AVAILABLE}
Apr 07, 2024 3:27:48 AM org.glassfish.jersey.server.wadl.WadlFeature configure
WARNING: JAXBContext implementation could not be found. WADL feature is disabled.
2024-04-07T03:27:48,377+0000 [main] INFO  org.eclipse.jetty.server.handler.ContextHandler - Started o.e.j.s.ServletContextHandler@420cd102{/proxy-stats,null,AVAILABLE}
2024-04-07T03:27:48,378+0000 [main] INFO  org.eclipse.jetty.server.handler.ContextHandler - Started o.e.j.s.ServletContextHandler@474179fa{/metrics,null,AVAILABLE}
2024-04-07T03:27:48,410+0000 [main] INFO  org.eclipse.jetty.server.handler.ContextHandler - Started o.e.j.s.ServletContextHandler@2abc8034{/admin,null,AVAILABLE}
2024-04-07T03:27:48,411+0000 [main] INFO  org.eclipse.jetty.server.handler.ContextHandler - Started o.e.j.s.ServletContextHandler@467b0f6e{/lookup,null,AVAILABLE}
2024-04-07T03:27:48,416+0000 [main] INFO  org.ecli
2024-04-07T11:27:48,613 - INFO  - [docker-java-stream-1508499096:DockerUtils$4@383] - DOCKER.exec(ClientToolTest-nwtuv-pulsar-proxy:tail -f /var/log/pulsar/proxy.log): STDOUT: pse.jetty.server.AbstractConnector - Started ServerConnector@2881ad47{HTTP/1.1, (http/1.1)}{0.0.0.0:8080}
2024-04-07T03:27:48,416+0000 [main] INFO  org.eclipse.jetty.server.Server - Started @4010ms
2024-04-07T03:27:48,417+0000 [main] INFO  org.apache.pulsar.proxy.server.WebServer - Server started at end point http://0.0.0.0:8080

My script

unzip -p pulsar-java-test-image.zst.zip | unzstd | docker load
export PULSAR_TEST_IMAGE_NAME=apachepulsar/java-test-image:latest
./build/run_integration_group.sh CLI -T 1C

@nodece nodece marked this pull request as draft April 7, 2024 14:50
@nodece
Copy link
Member Author

nodece commented Apr 7, 2024

slf4j seems to have some issues, and once the issues are fixed, I will continue working.

Issue: qos-ch/slf4j#410

@nodece nodece marked this pull request as ready for review April 8, 2024 02:28
@nodece
Copy link
Member Author

nodece commented Apr 8, 2024

/pulsarbot rerun-failure-checks

@nodece
Copy link
Member Author

nodece commented Apr 8, 2024

/pulsarbot rerun-failure-checks

1 similar comment
@nodece
Copy link
Member Author

nodece commented Apr 8, 2024

/pulsarbot rerun-failure-checks

pom.xml Outdated Show resolved Hide resolved
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece changed the title [improve][misc] Upgrade slf4j to 2.0.12 [improve][misc] Upgrade slf4j to 2.0.13 Apr 23, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece requested a review from lhotari April 24, 2024 03:01
@nodece nodece added this to the 3.3.0 milestone Apr 24, 2024
@nodece nodece requested a review from Technoboy- April 24, 2024 03:01
@nodece nodece merged commit 0c097ef into apache:master Apr 25, 2024
49 of 51 checks passed
@BewareMyPower
Copy link
Contributor

BewareMyPower commented Apr 26, 2024

It's a breaking change that affects the downstream.

In our KSN project (the private version of KoP), after upgrading to include this commit, logs cannot be printed anymore.

@BewareMyPower
Copy link
Contributor

I tried adding the reload4j dependency and it didn't work.

[INFO] \- org.slf4j:slf4j-reload4j:jar:2.0.13:test
[INFO]    \- ch.qos.reload4j:reload4j:jar:1.2.22:test
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation

@nodece
Copy link
Member Author

nodece commented Apr 26, 2024

Could you provide the log4j/log4j2 and slf4j versions?
I can help you solve the issues you are facing.

SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".

In your project, some libraries are using the slf4j 1.x, so you saw this log.

@nodece nodece added the release/important-notice The changes which are important should be mentioned in the release note label May 3, 2024
@mattisonchao
Copy link
Member

@nodece
Do we have any breaking changes for API/client API/client dependencies? we should raise a PIP and discuss it to decide if we have a breaking change. :)

cc @codelipenghui @BewareMyPower

@BewareMyPower
Copy link
Contributor

We should document explicitly that users should exclude slf4j-api from any pulsar-xxx dependency. Otherwise, users might suffer from such simple dependency upgrading.

In addition, for existing users, we should note this breaking change in 3.3.0's release note.

@mattisonchao
Copy link
Member

I mean we should discuss what strong reason pushed us to introduce this breaking change. :)

@BewareMyPower
Copy link
Contributor

BewareMyPower commented May 6, 2024

See my thread here: https://lists.apache.org/thread/4omvl62k4jhntj3rsywp14zzgh1l3l9q

For example, assuming users import the dependencies like:

<dependency>
  <groupId>org.apache.pulsar</groupId>
  <artifactId>pulsar-client-all</artifactId>
  <version>3.2.2</version>
</dependency>

<dependency>
  <groupId>org.slf4j</groupId>
  <artifactId>slf4j-simple</artifactId>
  <version>1.7.36</version>
</dependency>

After upgrading the pulsar-client-all dependency to 3.3.0, the slf4j-simple:1.7.36 dependency won't work. So if users want to upgrade the pulsar-client-all dependency, they have to exclude the slf4j-api dependency from pulsar-client-all.

@lhotari
Copy link
Member

lhotari commented May 6, 2024

We should document explicitly that users should exclude slf4j-api from any pulsar-xxx dependency. Otherwise, users might suffer from such simple dependency upgrading.

In addition, for existing users, we should note this breaking change in 3.3.0's release note.

I don't think that excluding is needed. Both maven and gradle use a dependency resolution strategy where the closest dependency wins when there are conflicts. The user would need to add slf4j-api dependency or use dependency management. Excluding isn't a recommended solution.

One could argue that the user already has a problem in the dependency management if they are relying on transitive dependencies for their slf4j-api dependency.

@lhotari
Copy link
Member

lhotari commented May 6, 2024

@nodece Do we have any breaking changes for API/client API/client dependencies? we should raise a PIP and discuss it to decide if we have a breaking change. :)

cc @codelipenghui @BewareMyPower

3.3.0 is for breaking changes like this. I explained in #22391 (comment) why the user already has a problem in dependency management if this breaks their application.

The world is moving forward and also other projects are moving towards slf4j-api 2.0.x . That will happen sooner or later for all projects.

It's better to make this change in 3.3.0 than in the 4.0.0 LTS version so that we have time to adjust things if needed for 4.0.0 . Introducing major changes in 4.0.0 isn't the best possible solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test release/important-notice The changes which are important should be mentioned in the release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants