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

Block shardingsphere-logging-core from passing logback-classic dependencies to downstream modules #32389

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

linghengqian
Copy link
Member

@linghengqian linghengqian commented Aug 4, 2024

Fixes #32377 .

Changes proposed in this pull request:

  • Block shardingsphere-logging-core from passing logback-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.
<dependency>
    <groupId>org.slf4j</groupId>
    <artifactId>slf4j-nop</artifactId>
    <version>1.7.36</version>
</dependency>

Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.

pom.xml Outdated
Comment on lines 990 to 993
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
</dependency>
Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
<scope>test</scope>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see from the dependency analyzer that the shardingsphere-standalone-mode-repository-jdbc module's dependency on logback-classic is already in test scope. Can you please help check it?

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • After I removed the declaration of ch.qos.logback:logback-classic here, I didn't see any logback dependencies. Without explicit declaration, the mode/type/standalone/repository/provider/jdbc/src/test/resources/logback-test.xml of this module will be useless. Where does this picture come from?
  • image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My screenshot is from here

image

Copy link
Member Author

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).

<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
<scope>test</scope>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the situation in shardingsphere-standalone-mode-repository-jdbc, it is already in test scope.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member Author

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) .

Copy link
Contributor

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?

Copy link
Contributor

@RaigorJiang RaigorJiang Aug 5, 2024

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        <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>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linghengqian Thank you!

@RaigorJiang RaigorJiang merged commit 455753d into apache:master Aug 5, 2024
142 checks passed
@linghengqian linghengqian deleted the fix-logback branch August 5, 2024 15:14
@linghengqian linghengqian added this to the 5.5.1 milestone Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shardingsphere-logging-core should not set the scope of logback-classic to compile
2 participants