From 592f63107ad8798018ee80d76b4d3ff0d5dad542 Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Fri, 16 May 2025 16:06:47 +0200 Subject: [PATCH] Fix `ColumnFamilyStore.getIfExists` handling of dropped keyspaces The `ColumnFamilyStore.getIfExists` method whole point is explained in its javadoc, which says: ``` Differently from others, this method does not throw exception if the table does not exist ``` That method work as expected if the table does not exists _but_ the keyspace does. But if the keyspace does not exists (in which case the table does not exists by extension), then it unexpectedly throws `UnknownKeyspaceException` instead of returning `null`. That is clearly unexpected as the code has the following snippet: ```java Keyspace keyspace = Keyspace.open(metadata.keyspace); if (keyspace == null) return null; ``` but this does not work as `Keyspace#open` never return `null`, it throws instead. This problem is years old, so it is obviously not a big issue. But at least in the context of CNDB, it can lead to unexpected errors in the log, and that is the source of some flaky tests. For instance, a recent run had a test failing with the following in the log: ``` [writer-0] DEBUG [grpc-default-executor-1] 2025-05-14 08:34:48,641 Schema.java:762 - Instance removed for keyspace testreadfromstorageservicewithpreparedidcollision [writer-0] DEBUG [grpc-default-executor-1] 2025-05-14 08:34:48,641 Schema.java:765 - Awaiting on write barrier before dropping keyspace testreadfromstorageservicewithpreparedidcollision [writer-0] DEBUG [StoragePeriodicReload:1] 2025-05-14 08:34:48,641 Keyspace.java:167 - New instance created for keyspace testreadfromstorageservicewithpreparedidcollision [writer-0] ERROR [StoragePeriodicReload:1] 2025-05-14 08:34:48,642 RemoteStorageHandler.java:339 - Failed periodic reload of sstables for testreadfromstorageservicewithpreparedidcollision.default_table with reason SSTABLES_CHANGED and attempts 0, rescheduling: [writer-0] org.apache.cassandra.exceptions.UnknownKeyspaceException: Could not find a keyspace testreadfromstorageservicewithpreparedidcollision [writer-0] at org.apache.cassandra.db.Keyspace.(Keyspace.java:368) [writer-0] at org.apache.cassandra.db.Keyspace.lambda$open$0(Keyspace.java:168) [writer-0] at org.apache.cassandra.utils.concurrent.LoadingMap.lambda$blockingLoadIfAbsent$1(LoadingMap.java:273) [writer-0] at org.apache.cassandra.utils.concurrent.LoadingMap.lambda$computeIfAbsent$0(LoadingMap.java:98) [writer-0] at org.apache.cassandra.utils.concurrent.LoadingMap.updateOrRemoveEntry(LoadingMap.java:178) [writer-0] at org.apache.cassandra.utils.concurrent.LoadingMap.computeIfAbsent(LoadingMap.java:98) [writer-0] at org.apache.cassandra.utils.concurrent.LoadingMap.blockingLoadIfAbsent(LoadingMap.java:272) [writer-0] at org.apache.cassandra.schema.Schema.maybeAddKeyspaceInstance(Schema.java:246) [writer-0] at org.apache.cassandra.db.Keyspace.open(Keyspace.java:166) [writer-0] at org.apache.cassandra.db.Keyspace.open(Keyspace.java:155) [writer-0] at org.apache.cassandra.db.ColumnFamilyStore.getIfExists(ColumnFamilyStore.java:3406) [writer-0] at com.datastax.cndb.metadata.storage.RemoteStorageHandler.maybeMarkIndexes(RemoteStorageHandler.java:1594) [writer-0] at com.datastax.cndb.metadata.storage.RemoteStorageHandler.doReloadSSTables(RemoteStorageHandler.java:1229) [writer-0] at com.datastax.cndb.metadata.storage.RemoteStorageHandler$Reloader.reload(RemoteStorageHandler.java:315) [writer-0] at com.datastax.cndb.metadata.storage.RemoteStorageHandler$Reloader.lambda$maybeSchedule$5(RemoteStorageHandler.java:406) [writer-0] at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572) [writer-0] at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317) [writer-0] at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) [writer-0] at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) [writer-0] at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) [writer-0] at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [writer-0] at java.base/java.lang.Thread.run(Thread.java:1570) ``` Here what happens is that the test keyspace is dropped by a `@AfterEach`, but this races with the periodic reloading of sstable which uses this `getIfExists` method. This patch fixes `ColumnFamilyStore.getIfExists` to return `null` if the keyspace does not exists. I will note that I reviewed all the usages of `ColumnFamilyStore.getIfExists` in CNDB. They are all of the form: ``` ColumnFamilyStore cfs = ColumnFamilyStore.getIfExists(...); if (cfs == null) return; ``` Meaning that this is used to not do some task if the table doesn't exists anymore. And so I'm confident this is the right change for CNDB. For the usages within C*, there is a number of uses cases where the code does: ``` ColumnFamilyStore cfs = ColumnFamilyStore.getIfExists(...); if (cfs == null) throw SomeException(...); ``` For such cases, this patch has the consequence that where previously a `UnknownKeyspaceException` was throw, now a `SomeException` will be throw instance. I assume this is fine. But in theory, some existing code could have been written to deal with `UnknownKeyspaceException` differently from `SomeException` somehow, and it would be really hard to follow every possible codepath upwards. Even if this were true, I can't imagine this have much consequences, so I'm reasonably confident with ignoring this risk, but wanted to mention it for completness. --- .../cassandra/db/ColumnFamilyStore.java | 4 ++-- .../org/apache/cassandra/db/Keyspace.java | 20 ++++++++++++++++- .../cassandra/db/ColumnFamilyStoreTest.java | 22 +++++++++++++++++++ .../cassandra/schema/SchemaTestUtil.java | 2 +- 4 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index 5efa62d9fde6..93b4f0f95827 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -3431,7 +3431,7 @@ public static ColumnFamilyStore getIfExists(TableId id) if (metadata == null) return null; - Keyspace keyspace = Keyspace.open(metadata.keyspace); + Keyspace keyspace = Keyspace.openIfExists(metadata.keyspace); if (keyspace == null) return null; @@ -3449,7 +3449,7 @@ public static ColumnFamilyStore getIfExists(String ksName, String cfName) if (ksName == null || cfName == null) return null; - Keyspace keyspace = Keyspace.open(ksName); + Keyspace keyspace = Keyspace.openIfExists(ksName); if (keyspace == null) return null; diff --git a/src/java/org/apache/cassandra/db/Keyspace.java b/src/java/org/apache/cassandra/db/Keyspace.java index 88873502036e..7ab8eeb9e0be 100644 --- a/src/java/org/apache/cassandra/db/Keyspace.java +++ b/src/java/org/apache/cassandra/db/Keyspace.java @@ -35,6 +35,8 @@ import java.util.function.Supplier; import java.util.stream.Stream; +import javax.annotation.Nullable; + import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Iterables; import com.google.common.util.concurrent.RateLimiter; @@ -149,7 +151,23 @@ public static void unsetInitialized() } } - public static Keyspace open(String keyspaceName) + /** + * Convenience function for when getting {@code null} if the keyspace does not exist is more convenient than a + * {@link UnknownKeyspaceException}. + */ + public static @Nullable Keyspace openIfExists(String keyspaceName) + { + try + { + return open(keyspaceName); + } + catch (UnknownKeyspaceException e) + { + return null; + } + } + + public static Keyspace open(String keyspaceName) throws UnknownKeyspaceException { assert initialized || SchemaConstants.isLocalSystemKeyspace(keyspaceName) : "Initialized: " + initialized; return open(keyspaceName, Schema.instance, true); diff --git a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java index 2a23d2ab13c9..45925ea3cd61 100644 --- a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java +++ b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java @@ -39,6 +39,8 @@ import org.apache.cassandra.db.partitions.Partition; import org.apache.cassandra.db.partitions.PartitionUpdate; import org.apache.cassandra.index.transactions.UpdateTransaction; +import org.apache.cassandra.schema.SchemaTestUtil; +import org.apache.cassandra.schema.TableMetadata; import org.apache.cassandra.utils.concurrent.OpOrder; import org.json.simple.JSONArray; import org.json.simple.JSONObject; @@ -803,4 +805,24 @@ public void performSnapshot(String snapshotName) } }; } + + /** + * Ensures `getIfExists` returns `null` (and does not throw) after a keyspace is dropped. + */ + @Test + public void testGetIfExistsAfterKeyspaceDrop() + { + String testKS = "getIfExistsAfterKeyspaceDrop"; + String testTable = "test_table"; + SchemaLoader.createKeyspace(testKS, + KeyspaceParams.simple(1), + SchemaLoader.standardCFMD(testKS, testTable)); + + TableMetadata metadata = ColumnFamilyStore.getIfExists(testKS, testTable).metadata.get(); + + SchemaTestUtil.announceKeyspaceDrop(testKS); + + assertNull(ColumnFamilyStore.getIfExists(metadata.id)); + assertNull(ColumnFamilyStore.getIfExists(testKS, testTable)); + } } diff --git a/test/unit/org/apache/cassandra/schema/SchemaTestUtil.java b/test/unit/org/apache/cassandra/schema/SchemaTestUtil.java index acc63078c8e1..f9242b1f6286 100644 --- a/test/unit/org/apache/cassandra/schema/SchemaTestUtil.java +++ b/test/unit/org/apache/cassandra/schema/SchemaTestUtil.java @@ -97,7 +97,7 @@ public static void announceTableUpdate(TableMetadata updated) Schema.instance.transform(schema -> schema.withAddedOrUpdated(ksm.withSwapped(ksm.tables.withSwapped(updated)))); } - static void announceKeyspaceDrop(String ksName) + public static void announceKeyspaceDrop(String ksName) { KeyspaceMetadata oldKsm = Schema.instance.getKeyspaceMetadata(ksName); if (oldKsm == null)