search

sofastack / sofa-registry

SOFARegistry is a production-level, low-latency, high-availability service registry powered by Ant Financial.
https://www.sofastack.tech/sofa-registry/docs/Home
Apache License 2.0
653 stars 247 forks source link

The usage of Log4j2 Strings Util class in the code makes users force dependency on log4j2. #345

Open aofall opened 12 months ago

aofall commented 12 months ago

In what area(s)?

/area test-and-release

https://github.com/sofastack/sofa-registry/blob/3d8297aae0017ed222e71add10f11bd2deeb23ec/server/server/session/src/main/java/com/alipay/sofa/registry/server/session/wrapper/BlacklistWrapperInterceptor.java#L31

https://github.com/sofastack/sofa-registry/blob/3d8297aae0017ed222e71add10f11bd2deeb23ec/server/server/integration/src/main/java/com/alipay/sofa/registry/server/integration/RegistryApplication.java#L45

Describe the feature

Please use the commons-lang3 library or maintain self StringUtils class in the project.

nocvalight commented 12 months ago

I agree with you, we can use commons-lang3 StringUtils to replaces Strings in log4j-api, and upgrade commons-lang to commons-lang3 in the project. Can you help to commit a pr to do this?

aofall commented 12 months ago

I agree with you, we can use commons-lang3 StringUtils to replaces Strings in log4j-api, and upgrade commons-lang to commons-lang3 in the project. Can you help to commit a pr to do this?

Done. PTAL

By the way, Is there a plan to publish new version to the Maven repository recently? The dubbo-spi-extension repository have some modules about sofa-registry and we has been doing version upgrading and writing unit tests. The is some problem encountered this issue when writing unit tests.

nocvalight commented 12 months ago

I agree with you, we can use commons-lang3 StringUtils to replaces Strings in log4j-api, and upgrade commons-lang to commons-lang3 in the project. Can you help to commit a pr to do this?

Done. PTAL

By the way, Is there a plan to publish new version to the Maven repository recently? The dubbo-spi-extension repository have some modules about sofa-registry and we has been doing version upgrading and writing unit tests. The is some problem encountered this issue when writing unit tests.

why use common-lang will cause problem when writing unit tests, what are the problems?

aofall commented 12 months ago

why use common-lang will cause problem when writing unit tests, what are the problems?

The main issue is the Strings reference to log4j. In the code left by previous developers, log4j was excluded and logback was introduced as an additional dependency.

https://github.com/apache/dubbo-spi-extensions/blob/9e99495f897493900f9b6ee2d961cf264484fd29/dubbo-registry-extensions/dubbo-registry-sofa/pom.xml#L101

It will cause java.lang.NoClassDefFoundError: org/apache/logging/log4j/spi/AbstractLoggerAdapter when used the TestRegistryMain.startRegistry() by the log4j-slf4j-impl, here is the stacktrace.

Failed to instantiate SLF4J LoggerFactory
Reported exception:
java.lang.NoClassDefFoundError: org/apache/logging/log4j/spi/AbstractLoggerAdapter
    at java.lang.ClassLoader.defineClass1(Native Method)
    at java.lang.ClassLoader.defineClass(ClassLoader.java:756)
    at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
    at java.net.URLClassLoader.defineClass(URLClassLoader.java:473)
    at java.net.URLClassLoader.access$100(URLClassLoader.java:74)
    at java.net.URLClassLoader$1.run(URLClassLoader.java:369)
    at java.net.URLClassLoader$1.run(URLClassLoader.java:363)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.net.URLClassLoader.findClass(URLClassLoader.java:362)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
    at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:355)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
    at org.slf4j.impl.StaticLoggerBinder.<clinit>(StaticLoggerBinder.java:36)
    at org.slf4j.LoggerFactory.bind(LoggerFactory.java:150)
    at org.slf4j.LoggerFactory.performInitialization(LoggerFactory.java:124)
    at org.slf4j.LoggerFactory.getILoggerFactory(LoggerFactory.java:412)
    at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:357)
    at com.alipay.sofa.registry.log.SLF4JLogger.getLoggerBySpace(SLF4JLogger.java:105)
    at com.alipay.sofa.registry.log.SLF4JLogger.<init>(SLF4JLogger.java:74)
    at com.alipay.sofa.registry.log.LoggerFactory.getLogger(LoggerFactory.java:42)
    at com.alipay.sofa.registry.net.NetUtil.<clinit>(NetUtil.java:44)
    at com.alipay.sofa.registry.server.test.TestRegistryMain.<clinit>(TestRegistryMain.java:35)
    at org.apache.dubbo.registry.sofa.HelloServiceImpl.main(HelloServiceImpl.java:37)

While excluded the log4j-slf4j-impl it will cause another NoClassDefFoundError, here is the stacktrace.

java.lang.NoClassDefFoundError: org/apache/logging/log4j/util/Strings
    at com.alipay.sofa.registry.server.integration.RegistryApplication.executeSqlScript(RegistryApplication.java:287) ~[registry-server-integration-6.3.0.jar:na]
    at com.alipay.sofa.registry.server.integration.RegistryApplication.createTables(RegistryApplication.java:217) ~[registry-server-integration-6.3.0.jar:na]
    at com.alipay.sofa.registry.server.integration.RegistryApplication.main(RegistryApplication.java:89) ~[registry-server-integration-6.3.0.jar:na]
    at com.alipay.sofa.registry.server.test.TestRegistryMain.startRegistry(TestRegistryMain.java:51) ~[registry-test-6.3.0.jar:na]
    at org.apache.dubbo.registry.sofa.HelloServiceImpl.main(HelloServiceImpl.java:39) ~[test-classes/:na]

I can certainly remove the exclusion statements, but I would recommend not force users dependency on log4j2. I just upgraded commons-lang to commons-lang3 based on this suggestion.

and upgrade commons-lang to commons-lang3 in the project.

However, some modules in the project still has the commons-lang library such as registry-server-data, which is transitive dependency by jraft-core.

If upgrade commons-lang to commons-lang3 is not necessary, I will close this PR and recreate another pr.