CASSJAVA-104: Fix Flaky tests by SiyaoIsHiding · Pull Request #2045 · apache/cassandra-java-driver

SiyaoIsHiding · 2025-07-02T23:40:13Z

Ready for review

SiyaoIsHiding · 2025-08-28T07:34:27Z

core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java

+  protected CqlPrepareAsyncProcessor(
+      Optional<? extends DefaultDriverContext> context, CacheBuilder<Object, Object> cacheBuilder) {
+    this.cache = cacheBuilder.build();
+    context.ifPresent(
+        (ctx) -> {
+          LOG.info("Adding handler to invalidate cached prepared statements on type changes");
+          EventExecutor adminExecutor = ctx.getNettyOptions().adminEventExecutorGroup().next();


@absurdfarce
CacheBuilder doesn't have a strongValues() method, so I guess our proposed solution about putting strongValues() to override the weak values wouldn't work.

Seems like there's an easier way to make that happen that doesn't involve introducing a parallel method to the one we've already exposed:

diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java b/core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java index a3d11cff0..7948931f0 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java @@ -64,14 +64,18 @@ public class CqlPrepareAsyncProcessor } public CqlPrepareAsyncProcessor(@NonNull Optional<? extends DefaultDriverContext> context) { - this(context, Functions.identity()); + // In the default case we mark our underlying cache as using weak values + this(context, builder->builder.weakValues()); } protected CqlPrepareAsyncProcessor( Optional<? extends DefaultDriverContext> context, Function<CacheBuilder<Object, Object>, CacheBuilder<Object, Object>> decorator) { - CacheBuilder<Object, Object> baseCache = CacheBuilder.newBuilder().weakValues(); + // Note that the base cache does NOT use weak values like the one-arg constructor above does! If + // you provide a decorator function for the CacheBuilder you must call weakValues() in that function + // if you want your cache to use weak values. + CacheBuilder<Object, Object> baseCache = CacheBuilder.newBuilder(); this.cache = decorator.apply(baseCache).build(); context.ifPresent( (ctx) -> {

absurdfarce

This looks really good to me; several great finds in here @SiyaoIsHiding! A couple of relatively minor changes I'd kinda like to see but overall I'm quite positive on where this PR is headed.

absurdfarce · 2026-03-04T22:11:03Z

core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java

+  protected CqlPrepareAsyncProcessor(
+      Optional<? extends DefaultDriverContext> context, CacheBuilder<Object, Object> cacheBuilder) {
+    this.cache = cacheBuilder.build();
+    context.ifPresent(
+        (ctx) -> {
+          LOG.info("Adding handler to invalidate cached prepared statements on type changes");
+          EventExecutor adminExecutor = ctx.getNettyOptions().adminEventExecutorGroup().next();


Seems like there's an easier way to make that happen that doesn't involve introducing a parallel method to the one we've already exposed:

diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java b/core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java index a3d11cff0..7948931f0 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java @@ -64,14 +64,18 @@ public class CqlPrepareAsyncProcessor } public CqlPrepareAsyncProcessor(@NonNull Optional<? extends DefaultDriverContext> context) { - this(context, Functions.identity()); + // In the default case we mark our underlying cache as using weak values + this(context, builder->builder.weakValues()); } protected CqlPrepareAsyncProcessor( Optional<? extends DefaultDriverContext> context, Function<CacheBuilder<Object, Object>, CacheBuilder<Object, Object>> decorator) { - CacheBuilder<Object, Object> baseCache = CacheBuilder.newBuilder().weakValues(); + // Note that the base cache does NOT use weak values like the one-arg constructor above does! If + // you provide a decorator function for the CacheBuilder you must call weakValues() in that function + // if you want your cache to use weak values. + CacheBuilder<Object, Object> baseCache = CacheBuilder.newBuilder(); this.cache = decorator.apply(baseCache).build(); context.ifPresent( (ctx) -> {

absurdfarce · 2026-03-04T22:29:58Z

...gration-tests/src/test/java/com/datastax/oss/driver/core/cql/PreparedStatementCachingIT.java

+        session.execute(
+            "CREATE TABLE test_table_caching_1 (e int primary key, f frozen<test_type_caching_1>)");
+        session.execute(
+            "CREATE TABLE test_table_caching_2 (g int primary key, h frozen<test_type_caching_2>)");


Good job, former me 🤦

Should the names of the tables and types created here not follow the basic/collection/tuple/nested split we use for setupCacheEntryTest* method names throughout? Seems like this should be:

session.execute("CREATE TYPE test_type_basic_1 (a text, b int)"); session.execute("CREATE TYPE test_type_basic_2 (c int, d text)"); session.execute("CREATE TABLE test_table_basic_1 (e int primary key, f frozen<test_type_1>)"); session.execute("CREATE TABLE test_table_basic_2 (g int primary key, h frozen<test_type_2>)");

and so on if we really want to avoid collisions.

invalidationResultSetTest, invalidationVariableDefsTest, and invalidationTestInner are shared among basic/collection/tuple/nested. They need to be refactored if we want to change the table names to specify basic/collection/tuple/nested. I don't think it's worth it, cuz test methods inside of one test suite won't run in parallel anyway

But don't we have to change the table name since these tests don't clean up after themselves? Once you execute the tests for, say, basic you've already created test_type_1, test_type_2 and all the rest. Won't you have a conflict then if you try to create those types against the same Cassandra backend (since they were never removed)?

This test suite has a @Rule instead of a @ClassRule, that means for every test method, CCM bridge creates a separate keyspace and session.

Wait, is that how that works? I remembered that working differently. My recollection was that CcmBridge just controlled the ccm interaction and that the session rule was the entity that constrained ops to a given keyspace.

Yeah, as I read this SessionRule can create a new keyspace if the user specifies it in the builder but this IT does not currently do so and this PR doesn't change that behaviour.

I do agree we need a new keyspace per test (since without that the create type calls will definitely stomp on each other) but I don't think we're quite there yet are we?

Actually, as I look at this again... why would the changes to this test do anything to fix the underlying problem? With the changes in this PR you still have exactly the same name collisions we had in the previous test... now everything just collides with test_table_caching or test_type_caching instead of test_table and test_type.

Setting aside the question of session isolation (which I still don't think we have right) why... is this better?

Oh, I get it now... the problem was that it was conflicting with UdtCodecIT, not that it was conflicting with the four variations of the test executed by this IT. 🤦

I completely misunderstood the problem... I'm good with this now.

absurdfarce · 2026-03-04T22:31:36Z

...on-tests/src/test/java/com/datastax/oss/driver/core/cql/PreparedStatementCancellationIT.java

+    public TestCqlPrepareAsyncProcessor(@NonNull Optional<DefaultDriverContext> context) {
+      // Default CqlPrepareAsyncProcessor uses weak values here as well.  We avoid doing so
+      // to prevent cache entries from unexpectedly disappearing mid-test.
+      super(context, CacheBuilder.newBuilder());


This would become:

super(context, Functions.identity())

given the change referenced above.

SiyaoIsHiding · 2026-03-05T07:34:58Z

PR updated except the "basic/collection/tuple/nested" in table names

tolbertam · 2026-03-23T15:45:44Z

osgi-tests/src/test/java/com/datastax/oss/driver/internal/osgi/support/BundleOptions.java


  public static UrlProvisionOption driverQueryBuilderBundle() {
-    return bundle("reference:file:../query-builder/target/classes");
+    return bundle("reference:file:../query-builder/target/classes").startLevel(2);


Guessing that deferring these later than the shaded bundle (and other deps) allows these to more reliably depend on the guava dep being there?

tolbertam · 2026-03-23T15:58:31Z

core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java

+    this(context, CacheBuilder::weakValues);
  }

  protected CqlPrepareAsyncProcessor(
      Optional<? extends DefaultDriverContext> context,
      Function<CacheBuilder<Object, Object>, CacheBuilder<Object, Object>> decorator) {

-    CacheBuilder<Object, Object> baseCache = CacheBuilder.newBuilder().weakValues();
+    // CASSJAVA-104
+    // Note that the base cache does NOT use weak values like the one-arg constructor it previously
+    // does!
+    CacheBuilder<Object, Object> baseCache = CacheBuilder.newBuilder();


Suggested change

this(context, CacheBuilder::weakValues);

}

protected CqlPrepareAsyncProcessor(

Optional<? extends DefaultDriverContext> context,

Function<CacheBuilder<Object, Object>, CacheBuilder<Object, Object>> decorator) {

CacheBuilder<Object, Object> baseCache = CacheBuilder.newBuilder().weakValues();

// CASSJAVA-104

// Note that the base cache does NOT use weak values like the one-arg constructor it previously

// does!

CacheBuilder<Object, Object> baseCache = CacheBuilder.newBuilder();

// Use weakValues to evict prepared statements from the cache as soon are they are

// no longer referenced elsewhere.

this(context, CacheBuilder::weakValues);

}

protected CqlPrepareAsyncProcessor(

Optional<? extends DefaultDriverContext> context,

Function<CacheBuilder<Object, Object>, CacheBuilder<Object, Object>> decorator) {

CacheBuilder<Object, Object> baseCache = CacheBuilder.newBuilder();

Was a little confused by this. It looks like the 1-argument constructor is used by the codebase, and the 2 argument constructor is used by tests, by moving the weakValues to the 1arg constructor, this allows the tests to work around the cache values being evicted as soon as they are no longer referenced elsewhere.

Think it would be better to just add a comment in the 1-arg constructor where weakValue is declared so it will be more clear why it's used there.

absurdfarce

Thanks (again) for taking this on @SiyaoIsHiding!

Similar to what I said on the CONTRIBUTING updates: I'm good with the state of this right now. Holding on giving an explicit 👍 until @tolbertam confirms that he's also good with everything as it stands.

SiyaoIsHiding · 2026-03-25T15:50:02Z

...ests/src/test/java/com/datastax/oss/driver/core/cql/reactive/DefaultReactiveResultSetIT.java

 import org.junit.rules.TestRule;
 import org.junit.runner.RunWith;

 @RunWith(DataProviderRunner.class)


To remind myself: add that @Category(ParallelizableTests.class) back

SiyaoIsHiding added 24 commits June 24, 2025 16:31

be generous for should_evict_down_node_metrics_when_timeout_fires

16b7883

mavenbundle start level 1

6f31d18

should_evict_down_node_metrics_when_timeout_fires to 40 seconds

ff0cc04

udt codec IT simple statement raise time out to 20 seconds

6594f3b

check schema agreement for PreparedStatementCachingIT

faeda6e

DefaultReactiveResultSetIT raise METADATA_SCHEMA_REQUEST_TIMEOUT?

935cf34

DefaultReactiveResultSetIT raise METADATA_SCHEMA_REQUEST_TIMEOUT?

ab7b1c1

change timeout to 120s or sth.

7ac257a

change table and type names

5593f5b

DefaultReactiveResultSetIT not paralell

646ffc9

empty

02dedff

compilation warning

e2b5062

empty

eb8822b

Merge remote-tracking branch 'upstream/4.x' into flaky-test

a7ff308

add logging

d7165f2

add logging

ec7a107

empty

cf054fa

empty

9045265

fmt

933bafb

error

6ec3c73

add logging

9ee5a8b

register listener first, create prepare later

3289fda

strong values of cache

98eb757

Merge remote-tracking branch 'upstream/4.x' into flaky-test

c0d2f74

SiyaoIsHiding changed the title ~~Flaky tests~~ CASSJAVA-104: Fix Flaky tests Jul 15, 2025

CancellationIT use strong values cache without cache removal callback

4f22bd6

SiyaoIsHiding force-pushed the flaky-test branch from 31c4c04 to 4f22bd6 Compare July 16, 2025 08:54

empty

9ba8203

SiyaoIsHiding commented Aug 28, 2025

View reviewed changes

tolbertam self-requested a review February 23, 2026 18:54

absurdfarce requested changes Mar 4, 2026

View reviewed changes

Address PR review

647d262

tolbertam reviewed Mar 23, 2026

View reviewed changes

tolbertam mentioned this pull request Mar 23, 2026

CASSJAVA-30: Update CONTRIBUTING.md #2041

Open

absurdfarce reviewed Mar 25, 2026

View reviewed changes

SiyaoIsHiding commented Mar 25, 2026

View reviewed changes

Conversation

SiyaoIsHiding commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

absurdfarce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

absurdfarce Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SiyaoIsHiding commented Mar 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

absurdfarce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SiyaoIsHiding commented Jul 2, 2025 •

edited

Loading

absurdfarce Mar 25, 2026 •

edited

Loading