-
Notifications
You must be signed in to change notification settings - Fork 6.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
Block shardingsphere-logging-core
from passing logback-classic
dependencies to downstream modules
#32389
Conversation
d0bd27b
to
be32c27
Compare
be32c27
to
16a0bd9
Compare
pom.xml
Outdated
<dependency> | ||
<groupId>ch.qos.logback</groupId> | ||
<artifactId>logback-classic</artifactId> | ||
</dependency> |
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.
@RaigorJiang What do you think about removing the test dependency of logback globally? Or keeping it?
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.
@linghengqian I'm in favor of removing it from the global, and temporarily disabling the logging rule, since it now binds to logback, and users don't know how to configure the rule.
#23856 provides a good idea, but before we implement the logging rule well, it is better not to let it affect users.
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.
So far, the current PR seems to be ready for the next review. In the current PR, Shardingsphere Proxy uses Logback with runtime scope, while ShardingSphere JDBC only provides Logback with provided scope.
.../native-image/org.apache.shardingsphere/generated-reachability-metadata/resource-config.json
Show resolved
Hide resolved
<dependency> | ||
<groupId>ch.qos.logback</groupId> | ||
<artifactId>logback-classic</artifactId> | ||
<scope>test</scope> |
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.
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.
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.
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.
It looks like you are looking at the dependencies of the master branch. On the master branch, logback in test scope is pulled into all modules, as I pointed out in #32389 (comment).
parser/sql/dialect/doris/pom.xml
Outdated
<dependency> | ||
<groupId>ch.qos.logback</groupId> | ||
<artifactId>logback-classic</artifactId> | ||
<scope>test</scope> |
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.
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.
- See Block
shardingsphere-logging-core
from passinglogback-classic
dependencies to downstream modules #32389 (comment) . After I removed the declaration ofch.qos.logback:logback-classic
here, I didn't see any logback dependencies. Without explicit declaration, theparser/sql/dialect/doris/src/test/resources/logback-test.xml
of this module will be useless. Where does this picture come from?
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.
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.
It looks like you are looking at the dependencies of the master branch. On the master branch, logback in test scope is pulled into all modules, as I pointed out in #32389 (comment) .
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.
@linghengqian Yes, I found the difference, in this PR you removed logback from the root pom and added it to specific modules, just to make logback not appear in the test scope downstream?
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.
Yes. Because if we do not exclude the test scope logback globally, after a module introduces Hiveserver2 JDBC Driver, multiple slf4j adapters including Reload4j, Log4j2, Logback, Jakarta Commons Logging, etc. will still appear in the classpath of the unit test at the same time, which violates https://www.slf4j.org/codes.html#multiple_bindings . I think a better solution is to explicitly declare Logback in the module that needs to use Logback. What do you think?
If we use <exclusion> to exclude incompatible dependencies when introducing new dependency, such as log4j introduced by hive, to ensure the simplicity of ShardingSphere itself, is this possible?
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.
- This is actually what I asked you at Block
shardingsphere-logging-core
from passinglogback-classic
dependencies to downstream modules #32389 (comment) . We have 2 options, one is to keep the test scope Logback globally, the other is to remove the test scope Logback globally. We can of course limit unit tests to use only Logback. If you think the test scope Log should be kept globally, another commit will be pushed.
<dependency>
<groupId>org.apache.hive</groupId>
<artifactId>hive-jdbc</artifactId>
<version>4.0.0</version>
</dependency>
<!-- See https://issues.apache.org/jira/browse/HIVE-28308 -->
<dependency>
<groupId>org.apache.hive</groupId>
<artifactId>hive-service</artifactId>
<version>4.0.0</version>
<exclusions>
<!-- See https://issues.apache.org/jira/browse/HIVE-28429 -->
<exclusion>
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-client-api</artifactId>
</exclusion>
<exclusion>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
</exclusion>
<exclusion>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
</exclusion>
<exclusion>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-client-api</artifactId>
<version>3.3.6</version>
</dependency>
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.
- This is actually what I asked you at Block
shardingsphere-logging-core
from passinglogback-classic
dependencies to downstream modules #32389 (comment) . We have 2 options, one is to keep the test scope Logback globally, the other is to remove the test scope Logback globally. We can of course limit unit tests to use only Logback. If you think the test scope Log should be kept globally, another commit will be pushed.
Sorry, I misunderstood your intention. I thought we were going to remove the logback dependency introduced by the logging rule.
For scenarios like hive, the <exclusion> method is more acceptable, and ShardingSphere developers can still focus on logback.
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.
- Done.
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.
@linghengqian Thank you!
…pendencies to downstream modules
16a0bd9
to
47cb400
Compare
Fixes #32377 .
Changes proposed in this pull request:
shardingsphere-logging-core
from passinglogback-classic
dependencies to downstream modules. Once merged, logging behavior for most modules will look the same as if the following dependencies were added. Unless the downstream module declares a declaration for some Slf4j implementation, such as Log4j1, Log4j2, Reload4j, Logback and the defunct Jakarta Commons Logging, which has been defunct for ten years. Of course Apache Commons Logging https://github.com/apache/commons-logging was recently resurrected.Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e
.