Skip to content

Commit 592f631

Browse files
committed
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.<init>(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.
1 parent 30f9d96 commit 592f631

File tree

4 files changed

+44
-4
lines changed

4 files changed

+44
-4
lines changed

src/java/org/apache/cassandra/db/ColumnFamilyStore.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3431,7 +3431,7 @@ public static ColumnFamilyStore getIfExists(TableId id)
34313431
if (metadata == null)
34323432
return null;
34333433

3434-
Keyspace keyspace = Keyspace.open(metadata.keyspace);
3434+
Keyspace keyspace = Keyspace.openIfExists(metadata.keyspace);
34353435
if (keyspace == null)
34363436
return null;
34373437

@@ -3449,7 +3449,7 @@ public static ColumnFamilyStore getIfExists(String ksName, String cfName)
34493449
if (ksName == null || cfName == null)
34503450
return null;
34513451

3452-
Keyspace keyspace = Keyspace.open(ksName);
3452+
Keyspace keyspace = Keyspace.openIfExists(ksName);
34533453
if (keyspace == null)
34543454
return null;
34553455

src/java/org/apache/cassandra/db/Keyspace.java

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535
import java.util.function.Supplier;
3636
import java.util.stream.Stream;
3737

38+
import javax.annotation.Nullable;
39+
3840
import com.google.common.annotations.VisibleForTesting;
3941
import com.google.common.collect.Iterables;
4042
import com.google.common.util.concurrent.RateLimiter;
@@ -149,7 +151,23 @@ public static void unsetInitialized()
149151
}
150152
}
151153

152-
public static Keyspace open(String keyspaceName)
154+
/**
155+
* Convenience function for when getting {@code null} if the keyspace does not exist is more convenient than a
156+
* {@link UnknownKeyspaceException}.
157+
*/
158+
public static @Nullable Keyspace openIfExists(String keyspaceName)
159+
{
160+
try
161+
{
162+
return open(keyspaceName);
163+
}
164+
catch (UnknownKeyspaceException e)
165+
{
166+
return null;
167+
}
168+
}
169+
170+
public static Keyspace open(String keyspaceName) throws UnknownKeyspaceException
153171
{
154172
assert initialized || SchemaConstants.isLocalSystemKeyspace(keyspaceName) : "Initialized: " + initialized;
155173
return open(keyspaceName, Schema.instance, true);

test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@
3939
import org.apache.cassandra.db.partitions.Partition;
4040
import org.apache.cassandra.db.partitions.PartitionUpdate;
4141
import org.apache.cassandra.index.transactions.UpdateTransaction;
42+
import org.apache.cassandra.schema.SchemaTestUtil;
43+
import org.apache.cassandra.schema.TableMetadata;
4244
import org.apache.cassandra.utils.concurrent.OpOrder;
4345
import org.json.simple.JSONArray;
4446
import org.json.simple.JSONObject;
@@ -803,4 +805,24 @@ public void performSnapshot(String snapshotName)
803805
}
804806
};
805807
}
808+
809+
/**
810+
* Ensures `getIfExists` returns `null` (and does not throw) after a keyspace is dropped.
811+
*/
812+
@Test
813+
public void testGetIfExistsAfterKeyspaceDrop()
814+
{
815+
String testKS = "getIfExistsAfterKeyspaceDrop";
816+
String testTable = "test_table";
817+
SchemaLoader.createKeyspace(testKS,
818+
KeyspaceParams.simple(1),
819+
SchemaLoader.standardCFMD(testKS, testTable));
820+
821+
TableMetadata metadata = ColumnFamilyStore.getIfExists(testKS, testTable).metadata.get();
822+
823+
SchemaTestUtil.announceKeyspaceDrop(testKS);
824+
825+
assertNull(ColumnFamilyStore.getIfExists(metadata.id));
826+
assertNull(ColumnFamilyStore.getIfExists(testKS, testTable));
827+
}
806828
}

test/unit/org/apache/cassandra/schema/SchemaTestUtil.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ public static void announceTableUpdate(TableMetadata updated)
9797
Schema.instance.transform(schema -> schema.withAddedOrUpdated(ksm.withSwapped(ksm.tables.withSwapped(updated))));
9898
}
9999

100-
static void announceKeyspaceDrop(String ksName)
100+
public static void announceKeyspaceDrop(String ksName)
101101
{
102102
KeyspaceMetadata oldKsm = Schema.instance.getKeyspaceMetadata(ksName);
103103
if (oldKsm == null)

0 commit comments

Comments
 (0)