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

[SNOW-859796] Get rid of SFL4J logging messages #1474

Merged
merged 4 commits into from
Jul 21, 2023

Conversation

sfc-gh-skumbham
Copy link
Contributor

@sfc-gh-skumbham sfc-gh-skumbham commented Jul 17, 2023

Overview

SNOW-859796

Even though this change looks simple - this introduces a behavior change on how developers are going to use snowflake-jdbc package and sl4j logging library.

Users using build tools such as Maven/Gradle etc.

This change makes the Sl4J-api as a compile-time dependency and removes the SL4J-API from the snowflake-jdbc jar. However, when the user uses any build tools such as maven, gradle etc., sl4j-api will be shown as depedency for snowflake jar as below:

Screenshot 2023-07-17 at 4 59 18 PM

Note log: Excluding org.slf4j:slf4j-api: jar:2.0.6 from the shaded jar.

[INFO] org.example:test-sdk:jar:1.0-SNAPSHOT
[INFO] \- net.snowflake:snowflake-jdbc:jar:3.13.45-SNAPSHOT:compile
[INFO]    \- org.slf4j:slf4j-api:jar:2.0.6:compile
[INFO] ------------------------------------------------------------------------

Building the Java application will download the slf4j-api jar as it is the dependency for the snowflake-jdbc and places it in the classpath.
This resolves issues: https://github.com/snowflakedb/snowflake-sdks-drivers-issues-teamwork/issues/497

With this fix users will start to see the following warning message if sl4j implementation is not found in the classpath:

SLF4J: No SLF4J providers were found.
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See https://www.slf4j.org/codes.html#noProviders for further details.

Users using the downloaded jar:

If user apps directly reference snowflake-jdbc.jar, then slf4j-api won't be downloaded or available in the classpath. Applications will start to fail with class not found exceptions:

SEVERE: Exception creating result
java.lang.NoClassDefFoundError: org/slf4j/LoggerFactory
        at net.snowflake.client.jdbc.internal.apache.arrow.memory.BaseAllocator.<clinit>(BaseAllocator.java:46)
        at net.snowflake.client.jdbc.SnowflakeResultSetSerializableV1.create(SnowflakeResultSetSerializableV1.java:591)
        at net.snowflake.client.jdbc.SnowflakeResultSetSerializableV1.create(SnowflakeResultSetSerializableV1.java:490)
        at net.snowflake.client.core.SFResultSetFactory.getResultSet(SFResultSetFactory.java:34)
        at net.snowflake.client.core.SFStatement.executeQueryInternal(SFStatement.java:227)
        at net.snowflake.client.core.SFStatement.executeQuery(SFStatement.java:133)
        at net.snowflake.client.core.SFStatement.execute(SFStatement.java:767)
        at net.snowflake.client.core.SFStatement.execute(SFStatement.java:676)
        at net.snowflake.client.jdbc.SnowflakeStatementV1.executeQueryInternal(SnowflakeStatementV1.java:274)
        at net.snowflake.client.jdbc.SnowflakeStatementV1.executeQuery(SnowflakeStatementV1.java:140)
        at GithubIssue1442.main(GithubIssue1442.java:42)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:568)
        at jdk.compiler/com.sun.tools.javac.launcher.Main.execute(Main.java:419)
        at jdk.compiler/com.sun.tools.javac.launcher.Main.run(Main.java:192)
        at jdk.compiler/com.sun.tools.javac.launcher.Main.main(Main.java:132)
Caused by: java.lang.ClassNotFoundException: org.slf4j.LoggerFactory

The only way to resolve this would be to place the sl4j-api.jar in the classpath.

Resolving SLF4J: No SLF4J providers were found

To resolve this, Users need to provide any slf4j implementation in the classpath.

SFSQL Tests:
Without slf4j jars in the classpath:
Screenshot 2023-07-17 at 5 04 23 PM

Screenshot 2023-07-17 at 5 04 23 PM

Screenshot 2023-07-17 at 5 05 03 PM

With slf4j jars:

Screenshot 2023-07-17 at 5 53 29 PM

External contributors - please answer these questions before submitting a pull request. Thanks!

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes #NNNN

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    Please write a short description of how your code change solves the related issue.

Pre-review checklist

  • This change has passed precommit
  • I have reviewed code coverage report for my PR in (Sonarqube)

@sfc-gh-skumbham sfc-gh-skumbham requested a review from a team as a code owner July 17, 2023 19:24
Copy link

@sfc-gh-pbennes sfc-gh-pbennes left a comment

Choose a reason for hiding this comment

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

slf4j-api as a regular dependency LGTM

@sfc-gh-igarish
Copy link
Collaborator

Yes we need to change FIPS XML. Make sure we can test in all known cases:

  1. BPTP henplus test
  2. BPTP henplus test with redirection
  3. Custom SLF4J logging using -D option
  4. No class found issue of JDK17
  5. Without custom SLF4J logging but if slf4 classes in PATH shouldn't spill "No slf4J provider" found log entry and similar entry at start.

Copy link
Collaborator

@sfc-gh-igarish sfc-gh-igarish left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sfc-gh-mknister sfc-gh-mknister left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@sfc-gh-pbennes sfc-gh-pbennes left a comment

Choose a reason for hiding this comment

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

Oh right, while Srikanth and I were going over this I realized we want to exclude slf4j-api from shading completely and not shade it with an exclusion for it's unrelocated shading in check_content.sh.

Srikanth is going to test this approach and report back any issues.

Marking this as request changes so we don't accidentally merge it as-is.

Copy link

@sfc-gh-pbennes sfc-gh-pbennes left a comment

Choose a reason for hiding this comment

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

PR LGTM

Copy link
Collaborator

@sfc-gh-igarish sfc-gh-igarish left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for making this change.

@sonarqubemergegate
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sfc-gh-skumbham sfc-gh-skumbham merged commit 1fa0074 into master Jul 21, 2023
@sfc-gh-skumbham sfc-gh-skumbham deleted the skumbham-SNOW-859796 branch July 21, 2023 19:16
@github-actions github-actions bot locked and limited conversation to collaborators Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants