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

Jump to bottom

Rethink Guava dependency in Java driver #1983

Open
wants to merge 3 commits into
base: 4.x
Choose a base branch
from
Open

Rethink Guava dependency in Java driver #1983

wants to merge 3 commits into from
+1,498 −960

Conversation

lukasz-antoniak
Copy link
Member

@lukasz-antoniak lukasz-antoniak commented Nov 6, 2024 •
edited
Loading

Fixes CASSJAVA-62.

adutra
adutra reviewed Nov 12, 2024
View reviewed changes
distribution/src/assembly/binary-tarball.xml Outdated
@@ -66,8 +66,8 @@
<exclude>org.apache.cassandra:java-driver-core</exclude>
<exclude>org.apache.cassandra:java-driver-mapper-runtime</exclude>
<exclude>org.apache.cassandra:java-driver-mapper-processor</exclude>
<exclude>org.apache.cassandra:java-driver-guava-shaded</exclude>
Copy link
Contributor

Choose a reason for hiding this comment

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

Manually verified the tar.gz, looks good.

guava-shaded/src/assembly/shaded-jar.xml Outdated
limitations under the License.

-->
<assembly xmlns="http://maven.apache.org/ASSEMBLY/2.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/ASSEMBLY/2.0.0 http://maven.apache.org/xsd/assembly-2.0.0.xsd">
Copy link
Contributor

Choose a reason for hiding this comment

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

Manually inspected the shaded jar, also looking good.

pom.xml Outdated Show resolved Hide resolved
@lukasz-antoniak lukasz-antoniak mentioned this pull request Nov 14, 2024
Open
emerkle826
emerkle826 approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@emerkle826 emerkle826 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 good to me

@absurdfarce
Copy link
Contributor

absurdfarce commented Nov 18, 2024 •
edited
Loading

So, I think this is fine as it stands, and it appears to accomplish the goal we want but I can't help but wonder... do we really even need this JAR anymore? Is there a clear argument for including this shaded JAR vs. just including (as a shaded JAR or as a stated dependency) Guava directly?

The above may be a dumb question; it's quite possible there's some aspect of this I'm not seeing. But as I look through this code I can't help but think that we're going to a lot of trouble to create a dependency from a shaded Guava JAR which seems... like something we could do without all that trouble.

absurdfarce
absurdfarce reviewed Nov 18, 2024
View reviewed changes
bom/pom.xml Outdated
<dependency>
<groupId>org.apache.cassandra</groupId>
<artifactId>java-driver-guava-shaded</artifactId>
<version>4.18.2-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

So we're versioning the shaded JAR based on... the driver release, which means we need to release a new version with every driver release... whether we've upgraded Guava or not? This seems... strange.

@absurdfarce
Copy link
Contributor

Based on conversation in ASF Slack it sounds like this might be moving in the direction of adding Guava to core-shaded and just making Guava a straight dependency for the non-shaded build. This creates two very distinct configurations: one with zero shading (the core JAR) and one with multiple major dependencies shaded (core-shaded). If users experience conflicts with the Guava version in use in their application and what the Java driver needs we can recommend those users move to core-shaded.

The analysis above is not settled yet, however, so this could still change.

lukasz-antoniak and others added 2 commits November 20, 2024 10:43
@lukasz-antoniak @adutra
Move DataStax shaded Guava module into Java driver
07cc8f0 
patch by Lukasz Antoniak; reviewed by Alexandre Dutra and Erik Merkle for CASSJAVA-52

Update pom.xml

Co-authored-by: Alexandre Dutra <[email protected]>
@lukasz-antoniak
Declare Guava as direct core module dependency
7641f18
@lukasz-antoniak
Copy link
Member Author

Declared Guava as a dependency to core module. Changes pushed.

@absurdfarce, @adutra: I still have a problem with OsgiShadedIT. Our OSGi test project (src/main/java/com/datastax/oss/driver/api/osgi) uses mapper and query builder. Those two modules do not have shaded versions and they depend on Guava (mapper runtime could skip Guava dependency, but inside query builder it is more widely used). Previously they both included shaded Guava as a dependency. I could not remove Guava bundle from OsgiShadedIT. Tested locally that when I remove all dependencies for mapper runtime and query builder from test project, shaded core driver does not require Guava. I have tried to implement a new test and use Pax Exam to create OSGi bundle from only part of test project (include only classes that do not import com.datastax.oss.driver.api.mapper), but not sure if it is possible. We could create a test like this with completely new Maven module, but not sure if it is worth the effort. What is you view on the issue?

@lukasz-antoniak
Copy link
Member Author

Checking if TinyBundles will do the job.

@lukasz-antoniak
Copy link
Member Author

I ended up implementing TweetService that does not use mapper and query builder. BundleOptions.applicationCoreBundle() creates also a small OSGi bundle with just Tweet service and its dependencies (skipping MailboxService and its imports). Test infrastructure is brought by BundleOptions.testBundles() and it also uses Guava. I have refactored parts of test-infra classes not to use Guava internally.

@lukasz-antoniak
Copy link
Member Author

I am changing the title of this PR to later rethink handling he dependency of Guava in the Java driver. All details have been captured in CASSJAVA-62. Let us wait with this PR until we are allowed to do a potentially breaking change. This PR includes some valuable work to use Guava as standard dependency (mainly one OSGi test).

@lukasz-antoniak lukasz-antoniak changed the title Move DataStax shaded Guava module into Java driver Rethink Guava dependency in Java driver Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@absurdfarce absurdfarce absurdfarce left review comments

@emerkle826 emerkle826 emerkle826 approved these changes

@adutra adutra adutra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lukasz-antoniak @absurdfarce @adutra @emerkle826