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

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

Open
aofall opened this issue Dec 2, 2023 · 4 comments

Comments

@aofall
Copy link

aofall commented Dec 2, 2023

In what area(s)?

/area test-and-release

Describe the feature

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

@aofall aofall changed the title The usage of Log4j2 Strings Util class in the code makes users dependency on log4j2. The usage of Log4j2 Strings Util class in the code makes users force dependency on log4j2. Dec 2, 2023
@nocvalight
Copy link
Member

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
Copy link
Author

aofall commented Dec 2, 2023

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
Copy link
Member

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
Copy link
Author

aofall commented Dec 3, 2023

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.

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 a pull request may close this issue.

2 participants