-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
[ISSUE #6906] Fix create NacosNamingService instance failure #6907
Conversation
|
Have you test the old version slf4j and logback can run well after your modified? |
I don't konw how to test this with |
@@ -33,15 +33,12 @@ | |||
|
|||
@Test | |||
public void testLoadConfiguration() { | |||
LoggerContext loggerContext = (LoggerContext) LogManager.getContext(false); | |||
Configuration contextConfiguration = loggerContext.getConfiguration(); | |||
Assert.assertEquals(0, contextConfiguration.getLoggers().size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this logic?
because nacos-client
dependency log4j
and logback
, but actual binding is of type [org.apache.logging.slf4j.Log4jLoggerFactory], and the binding is determined by the JVM, so when run mvn clean install -DskipITs
, the method NacosLogging.getInstance().loadConfiguration()
is executed in com.alibaba.nacos.client.utils.LogUtils
, and
contextConfiguration.getLoggers().size()
is more than one.
why it is work well before i modifed this issue? because effective logger is log4j, but determined logback, executed LogbackNacosLogging().loadConfiguration()
, contextConfiguration.getLoggers().size()
still zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But CI can pass, which means the old implementation is correct.
If can't remove this logic can't pass this case after your changes. We need more consider your changes whether has problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But CI can pass, which means the old implementation is correct.
If can't remove this logic can't pass this case after your changes. We need more consider your changes whether has problem.
ok, thanks for your reply, But I still think just executed Class.forName("ch.qos.logback.classic.Logger")
whthout exception, determined current logger instance is logback, is not particularly appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the PR provide a better one way to handle this problem.
But the problem is how to define the test case.
CLA not sign for a long time. |
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
Fix when dependency high version logback, low version slf4j, create NacosNamingService instance failure。
For #6906
Brief changelog
change NacosLogging determine logger type method
Verifying this change
when dependency logback,but effective logger is another logger implementation,for instance log4j,able to create NacosLoggin instance.
Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist
. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true
to make sure basic checks pass. Runmvn clean install -DskipITs
to make sure unit-test pass. Runmvn clean test-compile failsafe:integration-test
to make sure integration-test pass.