CASSJAVA-104: Fix Flaky tests

[SiyaoIsHiding]

Ready for review

[SiyaoIsHiding]

@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.

[absurdfarce]

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]

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]

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.

[SiyaoIsHiding]

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

[absurdfarce]

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)?

[SiyaoIsHiding]

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

[absurdfarce]

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?

[absurdfarce]

This would become:

   super(context, Functions.identity())

given the change referenced above.

[SiyaoIsHiding]

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