CASSJAVA-53 Upgrade Guava to 33.3.1-jre #1998

lukasz-antoniak · 2024-12-23T07:22:27Z

lukasz-antoniak · 2024-12-23T07:23:52Z

guava-shaded/pom.xml

@@ -78,6 +74,8 @@
                <includes>
                  <include>org.apache.cassandra:java-driver-guava-shaded</include>
                  <include>com.google.guava:guava</include>
+                  <include>com.google.guava:failureaccess</include>


New dependency introduced in Guava 27.0.

lukasz-antoniak · 2024-12-23T07:24:24Z

guava-shaded/pom.xml

@@ -78,6 +74,8 @@
                <includes>
                  <include>org.apache.cassandra:java-driver-guava-shaded</include>
                  <include>com.google.guava:guava</include>
+                  <include>com.google.guava:failureaccess</include>
+                  <include>com.google.j2objc:j2objc-annotations</include>


Required because SettableFuture extends AbstractFuture where the @ReflectionSupport(value = ReflectionSupport.Level.FULL) annotation is present.

That's... lame, but yeah, it appears to be the case.

lukasz-antoniak · 2024-12-23T07:26:22Z

guava-shaded/pom.xml

-          <groupId>org.codehaus.mojo</groupId>
-          <artifactId>animal-sniffer-annotations</artifactId>
+          <groupId>com.google.guava</groupId>
+          <artifactId>listenablefuture</artifactId>


ListenableFuture is already part of Guava in 27.0 and this is an optional dependency.

Is this strictly necessary? We're specifying excludes here, and per the blog post the listenablefuture dependency used for Guava is some weird high-numbered version (to avoid conflicts) which does indeed appear to be the case. If I'm understanding this correctly (and I'm not at all sure I am) we should be able to include the listenablefuture dependency at no real cost.

The only benefit that I can see for this approach is that it simplifies this POM a bit to not have a weird exclusion based on a weird Guava design choice.

Thoughts?

Based on this Guava issue, I guess that sequence of actions was the following:

Old Guava version contained ListenableFuture interface.

At some point Guava team decided to split parts of Guava into other libraries.

Later, it was decided to move the ListenableFuture interface back to Guava.

To prevent issues with dependency resolution, on some build systems, com.google.guava:listenablefuture was assigned a high version number and empty JAR, so that the com.google.guava:listenablefuture:1.0 is not picked up (which included colliding ListenableFuture interface definition).

I wanted to exclude it, because was thinking that driver users will not need to download extra JAR. However, since it is our shaded Guava version, it has no impact to driver users. I am OK removing this exclusion.

The blog post I referenced earlier provides a more complete picture of what happened @lukasz-antoniak:

Some Android library developers have told us that they would like to use ListenableFuture in their APIs, but they know that a full Guava dependency would be bad for apps that don't use Proguard. To address this, we are planning to release a separate artifact that contains only ListenableFuture.

I'm not quite sure I understand all of this; seems to me like you'd either want ListenableFuture by itself or Guava (with ListenableFuture still there)... which in turn means there's no real reason for Guava to depend on the listenablefuture lib. That change clearly was introduced to avoid seeing duplicate class definition errors... but I'm not sure how that's accomplished.

Regardless, I agree that removing the dependency should be harmless. For extra confirmation I confirmed that the high-numbered listenablefuture lib does indeed have no classes:

$ jar tf /work/maven/repo/com/google/guava/listenablefuture/9999.0-empty-to-avoid-conflict-with-guava/listenablefuture-9999.0-empty-to-avoid-conflict-with-guava.jar META-INF/MANIFEST.MF META-INF/ META-INF/maven/ META-INF/maven/com.google.guava/ META-INF/maven/com.google.guava/listenablefuture/ META-INF/maven/com.google.guava/listenablefuture/pom.xml META-INF/maven/com.google.guava/listenablefuture/pom.properties

I've got a final DS Jenkins run going with the most recent changes. Assuming there's no regression there (and I strongly suspect there won't be one) we're good to go on this one.

DS Jenkins run finished with a few test failures but they all look spurious. Should probably investigate these at some point in the future but I'm gonna hold off for now... if we start to see them more regularly I'll become more concerned.

lukasz-antoniak · 2024-12-23T07:27:25Z

guava-shaded/pom.xml

-        </exclusion>
-        <exclusion>
-          <groupId>org.codehaus.mojo</groupId>
-          <artifactId>animal-sniffer-annotations</artifactId>


Dependency animal-sniffer-annotations have removed from Guava 28.2.

adutra · 2024-12-26T11:14:55Z

core/src/test/java/com/datastax/dse/driver/internal/core/cql/reactive/TestSubscriber.java

@@ -17,7 +17,7 @@
 */
 package com.datastax.dse.driver.internal.core.cql.reactive;

-import static org.assertj.core.api.Fail.fail;
+import static org.junit.Assert.fail;


This change seems unrelated.

adutra · 2024-12-26T11:25:17Z

core/src/test/java/com/datastax/dse/driver/internal/core/cql/reactive/TestSubscriber.java

@@ -81,7 +81,10 @@ public List<T> getElements() {
  }

  public void awaitTermination() {
-    Uninterruptibles.awaitUninterruptibly(latch, 1, TimeUnit.MINUTES);
+    boolean interrupted = Uninterruptibles.awaitUninterruptibly(latch, 1, TimeUnit.MINUTES);


I believe this is incorrect. The returned value is the result of latch.await(), not a flag indicating that the thread was interrupted. The javadocs of CountDownLatch.wait() say:

return true if the count reached zero and false if the waiting time elapsed before the count reached zero.

Also looking at the implementation of awaitUninterruptibly in 33.3, there is already a finally block where the thread is being interrupted:

} finally { if (interrupted) { Thread.currentThread().interrupt(); } }

So I believe this code should be changed to:

if (!Uninterruptibles.awaitUninterruptibly(latch, 1, TimeUnit.MINUTES)) { fail("subscriber not terminated"); }

You are absolutely right. Not sure why I decided to change the code like this.

absurdfarce · 2025-01-03T21:07:31Z

I've kicked off a DS Jenkins build for this PR; will report results back when it's done.

absurdfarce · 2025-01-03T23:59:05Z

core/src/test/java/com/datastax/dse/driver/internal/core/cql/reactive/TestSubscriber.java

-    if (latch.getCount() > 0) fail("subscriber not terminated");
+    if (!Uninterruptibles.awaitUninterruptibly(latch, 1, TimeUnit.MINUTES)) {
+      fail("subscriber not terminated");
+    }


This seems correct. Uninterruptibles.awaitUninterruptibly() calls CountDownLatch.await() which (as I read the Javadoc) returns true if the latch count is equal to zero: in all other cases (including throwing an InterruptedException) execution continues until the timeout is hit which results in a return value of false.

absurdfarce · 2025-01-06T04:19:12Z

DS Jenkins test run was fine: one test failure but it looks to be completely unrelated (and caused by some ccm failure). So we're good to go on that front.

absurdfarce · 2025-01-06T17:38:05Z

@lukasz-antoniak Can you squash the commits on this PR into a single commit for me? Once that's done I think this guy is ready to merge.

patch by Lukasz Antoniak; reviewed by Alexandre Dutra and Bret McGuire for CASSJAVA-53

lukasz-antoniak · 2025-01-07T06:37:43Z

Thank you, squashed and commit message updated.

lukasz-antoniak commented Dec 23, 2024

View reviewed changes

adutra reviewed Dec 26, 2024

View reviewed changes

adutra approved these changes Dec 30, 2024

View reviewed changes

absurdfarce reviewed Jan 3, 2025

View reviewed changes

absurdfarce approved these changes Jan 6, 2025

View reviewed changes

Upgrade Guava to 33.3.1-jre

53b75ef

patch by Lukasz Antoniak; reviewed by Alexandre Dutra and Bret McGuire for CASSJAVA-53

lukasz-antoniak force-pushed the CASSJAVA-53 branch from a2c2346 to 53b75ef Compare January 7, 2025 06:37

absurdfarce merged commit 8c10099 into apache:4.x Jan 7, 2025
1 check failed

absurdfarce changed the title ~~Upgrade Guava to 33.3.1-jre~~ CASSJAVA-53 Upgrade Guava to 33.3.1-jre Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSJAVA-53 Upgrade Guava to 33.3.1-jre #1998

CASSJAVA-53 Upgrade Guava to 33.3.1-jre #1998

lukasz-antoniak commented Dec 23, 2024

lukasz-antoniak Dec 23, 2024

lukasz-antoniak Dec 23, 2024

absurdfarce Jan 4, 2025

lukasz-antoniak Dec 23, 2024

absurdfarce Jan 4, 2025

lukasz-antoniak Jan 6, 2025

absurdfarce Jan 6, 2025

absurdfarce Jan 6, 2025

lukasz-antoniak Dec 23, 2024

adutra Dec 26, 2024

lukasz-antoniak Dec 30, 2024

adutra Dec 26, 2024

lukasz-antoniak Dec 30, 2024

absurdfarce commented Jan 3, 2025

absurdfarce Jan 3, 2025

absurdfarce commented Jan 6, 2025

absurdfarce commented Jan 6, 2025

lukasz-antoniak commented Jan 7, 2025

CASSJAVA-53 Upgrade Guava to 33.3.1-jre #1998

CASSJAVA-53 Upgrade Guava to 33.3.1-jre #1998

Conversation

lukasz-antoniak commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absurdfarce commented Jan 3, 2025

Choose a reason for hiding this comment

absurdfarce commented Jan 6, 2025

absurdfarce commented Jan 6, 2025

lukasz-antoniak commented Jan 7, 2025