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.

Sign up for GitHub

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

JAVA-2982: Follow-up to recent PR making ESRI an optional dependency #1580

Merged
merged 6 commits into from
Jan 25, 2022
Merged

JAVA-2982: Follow-up to recent PR making ESRI an optional dependency #1580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9e43d52
Follow-up to recent PR making ESRI an optional dependency
absurdfarce Jan 7, 2022
130489c
Documentation updates
absurdfarce Jan 7, 2022
4c7e838
Added note re: use of older version of ESRI
absurdfarce Jan 21, 2022
5e9ce0d
Revising language of the Esri include version
absurdfarce Jan 21, 2022
953c72f
Remove transitive dependencies of Esri lib from shaded JAR as well
absurdfarce Jan 24, 2022
07293da
Removing OSGi spec for shaded packages which no longer exist
absurdfarce Jan 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 7 additions & 35 deletions core-shaded/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,6 @@
-->
<include>com.datastax.oss:java-driver-core</include>
<include>io.netty:*</include>
<include>com.esri.geometry:*</include>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit hesitant about this. Making a dependency optional and shading it are different concerns.

Initially, the decision to shade a dependency was taken by looking at the likelihood of a version conflict for a given dependency. For Esri, we deemed it high because the driver requires a very old Esri version.

I think we should keep shading Esri even if it is optional, wdyt? This means that in the shaded jar Esri would be present and shaded, just like it is now. I can't see a situation where this could be a problem for the end user.

Copy link
Contributor Author

@absurdfarce absurdfarce Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see your point here... I'm just not sure I agree.

To be clear, the case you're describing is fairly precise. It would have to be something like the following:

  • The user's app uses ESRI 2.x for some other functionality in their app
  • The user's app also uses the (shaded) Java driver
  • The user runs this app against DSE
  • The user wants to leverage the geometric type support in DSE

This user will (possibly, even likely) experience a class conflict. But the cost for supporting this (quite limited case) is that every user of the shaded JAR has a larger JAR on their classpath. Initially it seemed like optimizing for the broad case made sense rather than doing so for the extremely precise case but I'm certainly open to argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

every user of the shaded JAR has a larger JAR on their classpath

Well the shaded jar already has all of Netty in it, so adding Esri on top of it won't really make a noticeable size difference :-)

But OK, I don't mind if we stop shading Esri.

<include>org.json:*</include>
<include>org.codehaus.jackson:*</include>
<include>com.fasterxml.jackson.core:*</include>
</includes>
</artifactSet>
Expand All @@ -161,18 +158,6 @@
<pattern>io.netty</pattern>
<shadedPattern>com.datastax.oss.driver.shaded.netty</shadedPattern>
</relocation>
<relocation>
<pattern>com.esri</pattern>
<shadedPattern>com.datastax.oss.driver.shaded.esri</shadedPattern>
</relocation>
<relocation>
<pattern>org.json</pattern>
<shadedPattern>com.datastax.oss.driver.shaded.json</shadedPattern>
</relocation>
<relocation>
<pattern>org.codehaus.jackson</pattern>
<shadedPattern>com.datastax.oss.driver.shaded.codehaus.jackson</shadedPattern>
</relocation>
<relocation>
<pattern>com.fasterxml.jackson</pattern>
<shadedPattern>com.datastax.oss.driver.shaded.fasterxml.jackson</shadedPattern>
Expand All @@ -194,24 +179,6 @@
<exclude>META-INF/**</exclude>
</excludes>
</filter>
<filter>
<artifact>com.esri.geometry:*</artifact>
<excludes>
<exclude>META-INF/**</exclude>
</excludes>
</filter>
<filter>
<artifact>org.json:*</artifact>
<excludes>
<exclude>META-INF/**</exclude>
</excludes>
</filter>
<filter>
<artifact>org.codehaus.jackson:*</artifact>
<excludes>
<exclude>META-INF/**</exclude>
</excludes>
</filter>
<filter>
<artifact>com.fasterxml.jackson.core:*</artifact>
<excludes>
Expand Down Expand Up @@ -311,6 +278,11 @@
<artifactId>jctools-core</artifactId>
<version>2.1.2</version>
</additionalDependency>
<additionalDependency>
<groupId>com.esri.geometry</groupId>
<artifactId>esri-geometry-api</artifactId>
<version>1.2.1</version>
</additionalDependency>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

javadoc generation was throwing warnings about missing classes without this declaration in place

</additionalDependencies>
</configuration>
</execution>
Expand Down Expand Up @@ -340,7 +312,7 @@
<!--
1) Don't import packages shaded in the driver bundle. Note that shaded-guava lives
in its own bundle, so we must explicitly *not* mention it here.
-->!com.datastax.oss.driver.shaded.netty.*, !com.datastax.oss.driver.shaded.esri.*, !com.datastax.oss.driver.shaded.json.*, !com.datastax.oss.driver.shaded.codehaus.jackson.*, !com.datastax.oss.driver.shaded.fasterxml.jackson.*,
-->!com.datastax.oss.driver.shaded.netty.*, !com.datastax.oss.driver.shaded.fasterxml.jackson.*,
<!--
2) Don't include the packages below because they contain annotations only and are
not required at runtime.
Expand All @@ -366,7 +338,7 @@
1) The driver's packages (API and internal);
2) All shaded packages, except Guava which resides in a separate bundle.
-->
<Export-Package>com.datastax.oss.driver.api.core.*, com.datastax.oss.driver.internal.core.*, com.datastax.dse.driver.api.core.*, com.datastax.dse.driver.internal.core.*, com.datastax.oss.driver.shaded.netty.*, com.datastax.oss.driver.shaded.esri.*, com.datastax.oss.driver.shaded.json.*, com.datastax.oss.driver.shaded.codehaus.jackson.*, com.datastax.oss.driver.shaded.fasterxml.jackson.*,</Export-Package>
<Export-Package>com.datastax.oss.driver.api.core.*, com.datastax.oss.driver.internal.core.*, com.datastax.dse.driver.api.core.*, com.datastax.dse.driver.internal.core.*, com.datastax.oss.driver.shaded.netty.*, com.datastax.oss.driver.shaded.fasterxml.jackson.*,</Export-Package>
</instructions>
<rebuildBundle>true</rebuildBundle>
</configuration>
Expand Down
5 changes: 5 additions & 0 deletions integration-tests/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,11 @@
<artifactId>blockhound-junit-platform</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.esri.geometry</groupId>
<artifactId>esri-geometry-api</artifactId>
<scope>test</scope>
</dependency>
</dependencies>
<build>
<plugins>
Expand Down
21 changes: 19 additions & 2 deletions manual/core/integration/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,8 +479,9 @@ don't use any of the above features, you can safely exclude the dependency:
Our [geospatial types](../dse/geotypes/) implementation is based on the [Esri Geometry
API](https://github.com/Esri/geometry-api-java).

Esri is declared as a required dependency, but the driver can operate normally without it. If you
don't use geospatial types anywhere in your application, you can exclude the dependency:
For driver versions >= 4.4.0 and < 4.14.0 Esri is declared as a required dependency,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The TinkerPop example explicitly referred to an inclusive range, but in this case I wanted the range to be inclusive on the low end but exclusive on the high end (in order to explicitly convey the idea that all drivers before the named version used a different behaviour). Since this changed the semantics slightly I thought that perhaps identifying versions via the >= and < syntax might be more explicit.

I'm open to other ideas of presenting this information if this isn't clear.

although the driver can operate normally without it. If you don't use geospatial types
anywhere in your application you can exclude the dependency:

```xml
<dependency>
Expand All @@ -496,6 +497,22 @@ don't use geospatial types anywhere in your application, you can exclude the dep
</dependency>
```

Starting with driver 4.14.0 Esri has been changed to an optional dependency. You no longer have to
adutra marked this conversation as resolved.
Show resolved Hide resolved
explicitly exclude the dependency if it's not used, but if you do wish to make use of the Esri
library you must now explicitly specify it as a dependency :

```xml
<dependency>
<groupId>com.esri.geometry</groupId>
<artifactId>esri-geometry-api</artifactId>
<version>${esri.version}</version>
adutra marked this conversation as resolved.
Show resolved Hide resolved
</dependency>
```

In the dependency specification above you should use any 1.2.x version of Esri (we recommend
1.2.1). These versions are older than the current 2.x versions of the library but they are
guaranteed to be fully compatible with DSE.

#### TinkerPop

[Apache TinkerPop™](http://tinkerpop.apache.org/) is used in our [graph API](../dse/graph/),
Expand Down
20 changes: 20 additions & 0 deletions upgrade_guide/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,26 @@ request cannot be executed because all nodes tried were busy. Previously you wou
`NoNodeAvailableException` but you will now get back an `AllNodesFailedException` where the
`getAllErrors` map contains a `NodeUnavailableException` for that node.

#### Esri Geometry dependency now optional
adutra marked this conversation as resolved.
Show resolved Hide resolved

Previous versions of the Java driver defined a mandatory dependency on the Esri geometry library.
This library offered support for primitive geometric types supported by DSE. As of driver 4.14.0
this dependency is now optional.

If you do not use DSE (or if you do but do not use the support for geometric types within DSE) you
should experience no disruption. If you are using geometric types with DSE you'll now need to
explicitly declare a dependency on the Esri library:

```xml
<dependency>
<groupId>com.esri.geometry</groupId>
<artifactId>esri-geometry-api</artifactId>
<version>${esri.version}</version>
</dependency>
```

See the [integration](../manual/core/integration/#esri) section in the manual for more details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very much based on the example used when TinkerPop was made optional.

I've explicitly added this in the 4.14.0 section under the assumption that changing a dependency from required to optional breaks backwards compat and thus requires at least a minor release.

### 4.13.0

#### Enhanced support for GraalVM native images
Expand Down