-
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?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -469,6 +469,7 @@ | |
<configuration> | ||
<!-- The shaded uber jar is the default jar produced by this build --> | ||
<shadedArtifactAttached>false</shadedArtifactAttached> | ||
<createDependencyReducedPom>false</createDependencyReducedPom> | ||
<relocations> | ||
<relocation> | ||
<pattern>codegen</pattern> | ||
|
@@ -559,6 +560,44 @@ | |
</transformers> | ||
</configuration> | ||
</execution> | ||
|
||
</executions> | ||
</plugin> | ||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-install-plugin</artifactId> | ||
<version>3.1.1</version> | ||
Comment on lines
+571
to
+573
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
<executions> | ||
<!-- Skip default install to use our custom POMs --> | ||
<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 commentThe reason will be displayed to describe this comment to others. Learn more. nit: xml formatting |
||
<configuration> | ||
<skip>true</skip> | ||
</configuration> | ||
</execution> | ||
<!-- Install uber JAR with minimal POM (no dependencies) --> | ||
<execution> | ||
<id>install-uber</id> | ||
<phase>install</phase> | ||
<goals><goal>install-file</goal></goals> | ||
<configuration> | ||
<file>${project.build.directory}/${project.build.finalName}.jar</file> | ||
<pomFile>${project.basedir}/uber-minimal-pom.xml</pomFile> | ||
</configuration> | ||
</execution> | ||
<!-- Install thin JAR as separate artifact with different artifactId --> | ||
<execution> | ||
<id>install-thin</id> | ||
<phase>install</phase> | ||
<goals><goal>install-file</goal></goals> | ||
<configuration> | ||
<file>${project.build.directory}/${project.build.finalName}-thin.jar</file> | ||
<pomFile>${project.basedir}/thin_public_pom.xml</pomFile> | ||
<!-- No classifier - this is a separate artifact with different artifactId --> | ||
</configuration> | ||
</execution> | ||
</executions> | ||
</plugin> | ||
</plugins> | ||
|
@@ -595,10 +634,7 @@ | |
<configuration> | ||
<file>${project.build.directory}/${project.build.finalName}.jar</file> | ||
<pomFile>${project.basedir}/uber-minimal-pom.xml</pomFile> | ||
<!-- Include other artifacts like sources and javadocs --> | ||
<files>${project.build.directory}/${project.build.finalName}-thin.jar</files> | ||
<types>jar</types> | ||
<classifiers>thin</classifiers> | ||
<!-- Thin JAR will be deployed separately with different artifactId --> | ||
<!-- Use repository details from distributionManagement --> | ||
<repositoryId>local-test-repo</repositoryId> | ||
<url>file://${project.build.directory}/local-repo</url> | ||
|
@@ -663,13 +699,11 @@ | |
<arg>--pinentry-mode</arg> | ||
<arg>loopback</arg> | ||
</gpgArguments> | ||
<!-- Custom reduced pom file for fat jar --> | ||
<!-- Custom minimal pom file for uber JAR (no dependencies) --> | ||
<pomFile>${project.basedir}/uber-minimal-pom.xml</pomFile> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
<sources>${project.build.directory}/${project.build.finalName}-sources.jar</sources> | ||
<javadoc>${project.build.directory}/${project.build.finalName}-javadoc.jar</javadoc> | ||
<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 commentThe reason will be displayed to describe this comment to others. Learn more. Where is the thin jar being deployed? |
||
</configuration> | ||
</execution> | ||
</executions> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,177 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<project xmlns="http://maven.apache.org/POM/4.0.0" | ||
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:schemaLocation=" | ||
http://maven.apache.org/POM/4.0.0 | ||
http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
|
||
<modelVersion>4.0.0</modelVersion> | ||
<groupId>com.databricks</groupId> | ||
<artifactId>databricks-jdbc-thin</artifactId> | ||
<version>1.0.8-oss</version> | ||
<packaging>jar</packaging> | ||
<name>Databricks JDBC Driver - Thin JAR</name> | ||
<description>Databricks JDBC Driver.</description> | ||
<url>https://github.com/databricks/databricks-jdbc</url> | ||
|
||
<licenses> | ||
<license> | ||
<name>Apache License, Version 2.0</name> | ||
<url>https://github.com/databricks/databricks-jdbc/blob/main/LICENSE</url> | ||
</license> | ||
</licenses> | ||
|
||
<developers> | ||
<developer> | ||
<name>Databricks JDBC Team</name> | ||
<email>[email protected]</email> | ||
<organization>Databricks</organization> | ||
<organizationUrl>https://www.databricks.com</organizationUrl> | ||
</developer> | ||
</developers> | ||
|
||
<scm> | ||
<connection>scm:git:https://github.com/databricks/databricks-jdbc.git</connection> | ||
<developerConnection>scm:git:https://github.com/databricks/databricks-jdbc.git</developerConnection> | ||
<url>https://github.com/databricks/databricks-jdbc</url> | ||
</scm> | ||
|
||
<dependencies> | ||
<!-- 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 commentThe 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? |
||
<version>0.52.0</version> | ||
</dependency> | ||
<!-- Configuration --> | ||
<dependency> | ||
<groupId>org.apache.commons</groupId> | ||
<artifactId>commons-configuration2</artifactId> | ||
<version>2.10.1</version> | ||
</dependency> | ||
<!-- Apache Arrow --> | ||
<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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. absolutely, +1 |
||
<version>4.5.14</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.thrift</groupId> | ||
<artifactId>libthrift</artifactId> | ||
<version>0.19.0</version> | ||
</dependency> | ||
<!-- Logging API --> | ||
<dependency> | ||
<groupId>org.slf4j</groupId> | ||
<artifactId>slf4j-api</artifactId> | ||
<version>2.0.13</version> | ||
</dependency> | ||
<!-- Utilities --> | ||
<dependency> | ||
<groupId>commons-io</groupId> | ||
<artifactId>commons-io</artifactId> | ||
<version>2.14.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>com.google.code.findbugs</groupId> | ||
<artifactId>annotations</artifactId> | ||
<version>3.0.1</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>com.google.guava</groupId> | ||
<artifactId>guava</artifactId> | ||
<version>33.0.0-jre</version> | ||
</dependency> | ||
<!-- JSON and JWT --> | ||
<dependency> | ||
<groupId>com.fasterxml.jackson.core</groupId> | ||
<artifactId>jackson-core</artifactId> | ||
<version>2.18.3</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>com.fasterxml.jackson.core</groupId> | ||
<artifactId>jackson-databind</artifactId> | ||
<version>2.18.3</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>com.fasterxml.jackson.core</groupId> | ||
<artifactId>jackson-annotations</artifactId> | ||
<version>2.18.3</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>com.nimbusds</groupId> | ||
<artifactId>nimbus-jose-jwt</artifactId> | ||
<version>10.0.2</version> | ||
</dependency> | ||
<!-- Crypto --> | ||
<dependency> | ||
<groupId>org.bouncycastle</groupId> | ||
<artifactId>bcprov-jdk18on</artifactId> | ||
<version>1.78.1</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.bouncycastle</groupId> | ||
<artifactId>bcpkix-jdk18on</artifactId> | ||
<version>1.78.1</version> | ||
</dependency> | ||
<!-- LZ4, Netty, GRPC --> | ||
<dependency> | ||
<groupId>org.lz4</groupId> | ||
<artifactId>lz4-java</artifactId> | ||
<version>1.8.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>io.grpc</groupId> | ||
<artifactId>grpc-context</artifactId> | ||
<version>1.71.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>io.netty</groupId> | ||
<artifactId>netty-common</artifactId> | ||
<version>4.2.0.Final</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>io.netty</groupId> | ||
<artifactId>netty-buffer</artifactId> | ||
<version>4.2.0.Final</version> | ||
</dependency> | ||
<!-- Async HTTP (for Edge cases) --> | ||
<dependency> | ||
<groupId>org.apache.httpcomponents.client5</groupId> | ||
<artifactId>httpclient5</artifactId> | ||
<version>5.3.1</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.httpcomponents.core5</groupId> | ||
<artifactId>httpcore5</artifactId> | ||
<version>5.3.1</version> | ||
</dependency> | ||
<!-- Jakarta Annotations --> | ||
<dependency> | ||
<groupId>jakarta.annotation</groupId> | ||
<artifactId>jakarta.annotation-api</artifactId> | ||
<version>1.3.5</version> | ||
</dependency> | ||
</dependencies> | ||
</project> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add a new line |
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