-
Notifications
You must be signed in to change notification settings - Fork 18
[PECOBLR-747] add thin-public-pom.xml #930
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
base: main
Are you sure you want to change the base?
Conversation
@@ -664,7 +683,7 @@ | |||
<arg>loopback</arg> | |||
</gpgArguments> | |||
<!-- Custom reduced pom file for fat jar --> | |||
<pomFile>${project.basedir}/uber-minimal-pom.xml</pomFile> |
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.
do we need to remove the uber-minimal-pom in that case?
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.
Yeah, we should. I was just waiting for confirmation from @jayantsing-db
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.
hey, the purpose of minimal pom in the case of uber jar was that since the jar had all dependencies zipped in albeit shaded, we didn't want to load those dependencies again in customer env. this is for a variety of reasons:
- wasteful
- size of executable
- bootstrapping time
- unshaded dependencies creating conflicts with customer env
so if you were to use this thin pom. i see that it loads some critical dependencies which defeats the purpose stated above. so, for the fat jar, i would urge to keep the minimal pom
henceforth, i would recommend to create a separate artifact with thin
as suffix and publish the thin pom and thin jar as part of that artifact. hopefully, this makes sense.
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.
@jayantsing-db : I get your point. The reason we are looking to add a slim jar (other than the uber minimal) is because of competitor support and customer ask. But, we can choose not to support based on guidelines @shivam2680 Can you explore industry standards on the 3 type of JARs (especially on the uber minimal and slim jar).
thin_public_pom.xml
Outdated
|
||
<modelVersion>4.0.0</modelVersion> | ||
<groupId>com.databricks</groupId> | ||
<artifactId>databricks-jdbc</artifactId> |
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.
For my own understanding: how do we decide if a new dependency be added in the thin pom or not?
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.
everything except test-only and build-time dependencies.
scope=test, scope=provided
61926a9
to
24b4bca
Compare
pom.xml
Outdated
@@ -559,6 +560,44 @@ | |||
</transformers> | |||
</configuration> | |||
</execution> | |||
|
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.
nit: extraneous line
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-install-plugin</artifactId> | ||
<version>3.1.1</version> |
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.
Is this for local install? i see that the deploy is only signing and publishing the uber jar.
pom.xml
Outdated
<execution> | ||
<id>default-install</id> | ||
<phase>install</phase> | ||
<goals><goal>install</goal></goals> |
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.
nit: xml formatting
<version>1.3.5</version> | ||
</dependency> | ||
</dependencies> | ||
</project> |
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.
nit: add a new line
pom.xml
Outdated
<files>${project.build.directory}/${project.build.finalName}-thin.jar</files> | ||
<types>jar</types> | ||
<classifiers>thin</classifiers> | ||
<!-- Thin JAR will be released separately with different artifactId --> |
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.
Where is the thin jar being deployed?
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-install-plugin</artifactId> | ||
<version>3.1.1</version> |
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 this only locally installs the artifact and not publish to remote maven.
if it publishes (likely not the case now) to remote maven, we should cryptographically sign the artifact and pom
thin_public_pom.xml
Outdated
<dependency> | ||
<groupId>org.apache.arrow</groupId> | ||
<artifactId>arrow-memory-core</artifactId> | ||
<version>17.0.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.arrow</groupId> | ||
<artifactId>arrow-memory-unsafe</artifactId> | ||
<version>17.0.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.arrow</groupId> | ||
<artifactId>arrow-vector</artifactId> | ||
<version>17.0.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.arrow</groupId> | ||
<artifactId>arrow-memory-netty</artifactId> | ||
<version>17.0.0</version> | ||
</dependency> | ||
<!-- HTTP / Thrift --> | ||
<dependency> | ||
<groupId>org.apache.httpcomponents</groupId> | ||
<artifactId>httpclient</artifactId> |
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.
all of these dependencies version should ideally match the development pom? So people adding/updating new dependencies would have to remember to add/update the dependencies here too which creates a surface for human error. Can we leverage maven module/parent system to have one place for dependency version management?
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.
absolutely, +1
<!-- Core SDK --> | ||
<dependency> | ||
<groupId>com.databricks</groupId> | ||
<artifactId>databricks-sdk-java</artifactId> |
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 I don't see this being removed from the main pom? do we absolutely need all deps to be at 2 places?
Description
Implement proper POM metadata for the thin JAR artifact following industry standards to address dependency introspection limitations. This change enables users to view, pin, exclude, and override dependencies when using the thin JAR, matching industry standard approach with separate artifact IDs.
Testing
Build completes successfully
install phase succeeds without POM related errors
Both artifacts installed at
~/.m2/repository/com/databricks/databricks-jdbc/1.0.8-oss/
Fat JAR (Uber):
Thin JAR:
Create test project with thin jar dependency
Ran a sample test file to use the jar
Additional Notes to the Reviewer