absurdfarce commented on code in PR #2045:
URL:
https://github.com/apache/cassandra-java-driver/pull/2045#discussion_r2886404795
##########
core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java:
##########
@@ -83,6 +83,19 @@ protected CqlPrepareAsyncProcessor(
});
}
+ 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();
Review Comment:
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) -> {
```
##########
integration-tests/src/test/java/com/datastax/oss/driver/core/cql/PreparedStatementCachingIT.java:
##########
@@ -295,17 +298,20 @@ private void invalidationTestInner(
Consumer<CqlSession> setupCacheEntryTestBasic =
(session) -> {
- session.execute("CREATE TYPE test_type_1 (a text, b int)");
- session.execute("CREATE TYPE test_type_2 (c int, d text)");
- session.execute("CREATE TABLE test_table_1 (e int primary key, f
frozen<test_type_1>)");
- session.execute("CREATE TABLE test_table_2 (g int primary key, h
frozen<test_type_2>)");
+ session.execute("CREATE TYPE test_type_caching_1 (a text, b int)");
+ session.execute("CREATE TYPE test_type_caching_2 (c int, d text)");
+ 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>)");
Review Comment:
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.
##########
integration-tests/src/test/java/com/datastax/oss/driver/core/cql/PreparedStatementCancellationIT.java:
##########
@@ -50,6 +64,69 @@ public class PreparedStatementCancellationIT {
@Rule public TestRule chain =
RuleChain.outerRule(ccmRule).around(sessionRule);
+ private static class TestCqlPrepareAsyncProcessor extends
CqlPrepareAsyncProcessor {
+
+ 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());
Review Comment:
This would become:
```
super(context, Functions.identity())
```
given the change referenced above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
