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-1257851 Move to using snowjdbc thin jar instead of the fat jar #733

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

sfc-gh-alhuang
Copy link
Contributor

@sfc-gh-alhuang sfc-gh-alhuang marked this pull request as ready for review April 24, 2024 21:20
@sfc-gh-tzhang
Copy link
Contributor

@sfc-gh-lsembera @sfc-gh-alhuang I want to revisit our decision of NOT shading the JDBC dependency after this change, we have seen too many issues that customer is using an older version of JDBC with the latest SDK and something is not working due to bugs in the older versions of JDBC. Usually these issues are hard to debug as well since it requires JDBC logs and we don't know if the issues are in SDK or JDBC. I believe one of the main reason to not shading was due to the user jar size, which will be addressed in this PR, WDYT?

Copy link
Contributor

@sfc-gh-lsembera sfc-gh-lsembera left a comment

Choose a reason for hiding this comment

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

This looks fantastic, thanks @sfc-gh-alhuang! I added a few minor comments.

Since this is a huge PR, and it may have an impact on customers' setup, we should release it as 2.2.0. Before releasing, I'd also suggest to first upgrade everything internally to snapshot version from nexus to be 100% sure there are no regressions.

@@ -78,11 +84,31 @@
<type>pom</type>
<scope>import</scope>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to pull all these and not get them transitively via jdbc-thin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For com.amazonaws:aws-java-sdk-core, I got Used undeclared dependencies found error from maven-dependency-plugin if it's not explicitly specified in dependencies.

Copy link
Contributor Author

@sfc-gh-alhuang sfc-gh-alhuang May 3, 2024

Choose a reason for hiding this comment

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

For other dependencies like com.google.code.gson, Failed while enforcing releasability. error is raised by maven-enforcer-plugin if it's not specified in dependencies.

Rule 3: org.apache.maven.enforcer.rules.dependency.DependencyConvergence failed with message:
[ERROR] Failed while enforcing releasability.
[ERROR] 
[ERROR] Dependency convergence error for com.google.code.gson:gson:jar:2.8.9 paths to dependency are:
[ERROR] +-net.snowflake:snowflake-ingest-sdk:jar:2.1.1-SNAPSHOT
[ERROR]   +-net.snowflake:snowflake-jdbc-thin:jar:3.14.5:compile
[ERROR]     +-com.google.cloud:google-cloud-core:jar:2.21.0:compile
[ERROR]       +-com.google.protobuf:protobuf-java-util:jar:3.23.2:compile
[ERROR]         +-com.google.code.gson:gson:jar:2.8.9:compile
[ERROR] and
[ERROR] +-net.snowflake:snowflake-ingest-sdk:jar:2.1.1-SNAPSHOT
[ERROR]   +-net.snowflake:snowflake-jdbc-thin:jar:3.14.5:compile
[ERROR]     +-com.google.cloud:google-cloud-core:jar:2.21.0:compile
[ERROR]       +-com.google.http-client:google-http-client-gson:jar:1.43.3:compile
[ERROR]         +-com.google.code.gson:gson:jar:2.10.1:compile
[ERROR] and
[ERROR] +-net.snowflake:snowflake-ingest-sdk:jar:2.1.1-SNAPSHOT
[ERROR]   +-net.snowflake:snowflake-jdbc-thin:jar:3.14.5:compile
[ERROR]     +-com.google.cloud:google-cloud-storage:jar:2.22.6:compile
[ERROR]       +-com.google.code.gson:gson:jar:2.10.1:compile

@@ -61,6 +61,64 @@
"org.bouncycastle:bcpkix-jdk18on": BOUNCY_CASTLE_LICENSE,
"org.bouncycastle:bcutil-jdk18on": BOUNCY_CASTLE_LICENSE,
"org.bouncycastle:bcprov-jdk18on": BOUNCY_CASTLE_LICENSE,
"com.amazonaws:aws-java-sdk-core": APACHE_LICENSE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you check all of these don't ship with a license?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I checked the artifact directory to see if there's a license file. If not, I used the output from license:add-third-party directly.

@sfc-gh-lsembera
Copy link
Contributor

@sfc-gh-lsembera @sfc-gh-alhuang I want to revisit our decision of NOT shading the JDBC dependency after this change, we have seen too many issues that customer is using an older version of JDBC with the latest SDK and something is not working due to bugs in the older versions of JDBC. Usually these issues are hard to debug as well since it requires JDBC logs and we don't know if the issues are in SDK or JDBC. I believe one of the main reason to not shading was due to the user jar size, which will be addressed in this PR, WDYT?

Yes, this PR shades JDBC in the shaded JAR, the exclusion for snowflake-jdbc is gone, (see here).

@sfc-gh-japatel
Copy link
Collaborator

sfc-gh-japatel commented May 3, 2024

May be this is already discussed, but can we confirm from jdbc team this is okay to use in production?
https://docs.snowflake.com/en/release-notes/clients-drivers/jdbc-2024
release notes says, it is an experimental feature - not sure what it means -/ (PrPr?)

but if we have confirmed this is okay to use, please ignore my above comment.

@sfc-gh-tzhang
Copy link
Contributor

May be this is already discussed, but can we confirm from jdbc team this is okay to use in production? https://docs.snowflake.com/en/release-notes/clients-drivers/jdbc-2024 release notes says, it is an experimental feature - not sure what it means -/ (PrPr?)

but if we have confirmed this is okay to use, please ignore my above comment.

I believe we did check with them, but I didn't see the note, it's always good to double check again :)

@sfc-gh-tzhang
Copy link
Contributor

Thanks @sfc-gh-alhuang ! No concerns from me as long as @sfc-gh-lsembera signed off, could you wait for release 2.1.1 to go out before merging this change?

@sfc-gh-lsembera
Copy link
Contributor

@sfc-gh-alhuang @sfc-gh-tzhang Are we going to merge this one? If so, we should do it early, so we can internally switch to the new SNAPSHOT version, so it gets thoroughly tested before the release.

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 this pull request may close these issues.

4 participants