From c8ad005a04235d4b7b9b6b2f2c1bd297da96063a Mon Sep 17 00:00:00 2001 From: Arvind Kandpal Date: Mon, 18 May 2026 16:10:48 +0530 Subject: [PATCH 1/2] CASSANDRA-21133: Make bin/sstableupgrade functionally on par with nodetool upgradesstables --- .../tools/sstable/sstableupgrade.adoc | 1 + .../cassandra/db/ColumnFamilyStore.java | 21 +++++--- .../cassandra/tools/StandaloneUpgrader.java | 6 ++- .../cassandra/db/ColumnFamilyStoreTest.java | 50 +++++++++++++++++++ .../tools/StandaloneUpgraderTest.java | 18 +++++-- 5 files changed, 86 insertions(+), 10 deletions(-) diff --git a/doc/modules/cassandra/pages/managing/tools/sstable/sstableupgrade.adoc b/doc/modules/cassandra/pages/managing/tools/sstable/sstableupgrade.adoc index ad193e2f5a56..771f52fb39ec 100644 --- a/doc/modules/cassandra/pages/managing/tools/sstable/sstableupgrade.adoc +++ b/doc/modules/cassandra/pages/managing/tools/sstable/sstableupgrade.adoc @@ -25,6 +25,7 @@ sstableupgrade [snapshot_name] |=== |--debug |display stack traces |-h,--help |display this help message +|-a,--include-all-sstables |include all sstables, even those already on the current version |-k,--keep-source |do not delete the source sstables |=== diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index df862d3dc73d..7a6181401f63 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -1025,12 +1025,21 @@ public Descriptor newSSTableDescriptor(File directory, SSTableFormat forma public Descriptor newSSTableDescriptor(File directory, Version version) { - Descriptor newDescriptor = new Descriptor(version, - directory, - getKeyspaceName(), - name, - sstableIdGenerator.get()); - assert !newDescriptor.fileFor(Components.DATA).exists(); + Descriptor newDescriptor; + while (true) + { + newDescriptor = new Descriptor(version, + directory, + getKeyspaceName(), + name, + sstableIdGenerator.get()); + + if (!newDescriptor.fileFor(Components.DATA).exists()) + break; + + logger.warn("Generated SSTable id {} collides with existing file; advancing.", newDescriptor.id); + } + return newDescriptor; } diff --git a/src/java/org/apache/cassandra/tools/StandaloneUpgrader.java b/src/java/org/apache/cassandra/tools/StandaloneUpgrader.java index 069fdbe8451f..d8fc0b459a62 100644 --- a/src/java/org/apache/cassandra/tools/StandaloneUpgrader.java +++ b/src/java/org/apache/cassandra/tools/StandaloneUpgrader.java @@ -53,6 +53,7 @@ public class StandaloneUpgrader private static final String DEBUG_OPTION = "debug"; private static final String HELP_OPTION = "help"; private static final String KEEP_SOURCE = "keep-source"; + private static final String INCLUDE_ALL = "include-all-sstables"; public static void main(String args[]) { @@ -91,7 +92,7 @@ public static void main(String args[]) try { SSTableReader sstable = SSTableReader.openNoValidation(entry.getKey(), components, cfs); - if (sstable.descriptor.version.equals(DatabaseDescriptor.getSelectedSSTableFormat().getLatestVersion())) + if (!options.includeAll && sstable.descriptor.version.equals(DatabaseDescriptor.getSelectedSSTableFormat().getLatestVersion())) { sstable.selfRef().release(); continue; @@ -151,6 +152,7 @@ private static class Options public boolean debug; public boolean keepSource; + public boolean includeAll; private Options(String keyspace, String cf, String snapshot) { @@ -191,6 +193,7 @@ public static Options parseArgs(String cmdArgs[]) opts.debug = cmd.hasOption(DEBUG_OPTION); opts.keepSource = cmd.hasOption(KEEP_SOURCE); + opts.includeAll = cmd.hasOption(INCLUDE_ALL); return opts; } @@ -214,6 +217,7 @@ private static CmdLineOptions getCmdLineOptions() options.addOption(null, DEBUG_OPTION, "display stack traces"); options.addOption("h", HELP_OPTION, "display this help message"); options.addOption("k", KEEP_SOURCE, "do not delete the source sstables"); + options.addOption("a", "include-all-sstables", "include all sstables, even those already on the current version"); return options; } diff --git a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java index c8b636c94e28..ce49a50c3861 100644 --- a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java +++ b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java @@ -48,6 +48,7 @@ import org.apache.cassandra.SchemaLoader; import org.apache.cassandra.UpdateBuilder; import org.apache.cassandra.Util; +import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.cql3.ColumnIdentifier; import org.apache.cassandra.cql3.Operator; import org.apache.cassandra.cql3.statements.schema.IndexTarget; @@ -71,8 +72,11 @@ import org.apache.cassandra.index.transactions.UpdateTransaction; import org.apache.cassandra.io.sstable.Component; import org.apache.cassandra.io.sstable.Descriptor; +import org.apache.cassandra.io.sstable.SSTableIdFactory; +import org.apache.cassandra.io.sstable.SSTableId; import org.apache.cassandra.io.sstable.SSTableReadsListener; import org.apache.cassandra.io.sstable.ScrubTest; +import org.apache.cassandra.io.sstable.format.Version; import org.apache.cassandra.io.sstable.format.SSTableFormat.Components; import org.apache.cassandra.io.sstable.format.SSTableReader; import org.apache.cassandra.io.util.File; @@ -839,6 +843,52 @@ public static long getSnapshotManifestAndSchemaFileSizes(TableSnapshot snapshot) return schemaAndManifestFileSizes; } + @Test + public void testNewSSTableDescriptorCollision() throws Exception + { + ColumnFamilyStore cfs = Keyspace.open(KEYSPACE1).getColumnFamilyStore(CF_STANDARD1); + File dir = cfs.getDirectories().getDirectoryForNewSSTables(); + Version version = DatabaseDescriptor.getSelectedSSTableFormat().getLatestVersion(); + + Descriptor probe = cfs.newSSTableDescriptor(dir, version); + + int currentId; + try { + currentId = Integer.parseInt(probe.id.toString()); + } catch (NumberFormatException e) { + return; + } + + SSTableId blockedId1 = SSTableIdFactory.instance.fromString(String.valueOf(currentId + 1)); + SSTableId blockedId2 = SSTableIdFactory.instance.fromString(String.valueOf(currentId + 2)); + + Descriptor blockedDesc1 = new Descriptor(version, dir, cfs.getKeyspaceName(), cfs.name, blockedId1); + Descriptor blockedDesc2 = new Descriptor(version, dir, cfs.getKeyspaceName(), cfs.name, blockedId2); + + File blockedDataFile1 = blockedDesc1.fileFor(Components.DATA); + File blockedDataFile2 = blockedDesc2.fileFor(Components.DATA); + + try + { + blockedDataFile1.createFileIfNotExists(); + blockedDataFile2.createFileIfNotExists(); + + Descriptor nextDesc = cfs.newSSTableDescriptor(dir, version); + + Assert.assertNotEquals("The loop should have skipped the first blocked ID", + blockedDesc1.id, nextDesc.id); + Assert.assertNotEquals("The loop should have skipped the second blocked ID", + blockedDesc2.id, nextDesc.id); + Assert.assertFalse("The returned descriptor must not already exist on disk", + nextDesc.fileFor(Components.DATA).exists()); + } + finally + { + blockedDataFile1.deleteIfExists(); + blockedDataFile2.deleteIfExists(); + } + } + private Memtable fakeMemTableWithMinTS(ColumnFamilyStore cfs, long minTS) { return new AbstractMemtable(cfs.metadata, minTS) diff --git a/test/unit/org/apache/cassandra/tools/StandaloneUpgraderTest.java b/test/unit/org/apache/cassandra/tools/StandaloneUpgraderTest.java index 4d970eb22bf0..4e1bf1a6b21b 100644 --- a/test/unit/org/apache/cassandra/tools/StandaloneUpgraderTest.java +++ b/test/unit/org/apache/cassandra/tools/StandaloneUpgraderTest.java @@ -48,9 +48,11 @@ public void testMaybeChangeDocs() "hard links to live sstables.\n" + "--\n" + "Options are:\n" + - " --debug display stack traces\n" + - " -h,--help display this help message\n" + - " -k,--keep-source do not delete the source sstables\n"; + " -a,--include-all-sstables include all sstables, even those already on the\n" + + " current version\n" + + " --debug display stack traces\n" + + " -h,--help display this help message\n" + + " -k,--keep-source do not delete the source sstables\n"; Assertions.assertThat(tool.getStdout()).isEqualTo(help); } @@ -86,6 +88,16 @@ public void testFlagArgs() assertEquals(0, tool.getExitCode()); assertCorrectEnvPostTest(); }); + + Arrays.asList("-a", "--include-all-sstables").forEach(arg -> { + ToolResult tool = ToolRunner.invokeClass(StandaloneUpgrader.class, + arg, + "system_schema", + "tables"); + Assertions.assertThat(tool.getCleanedStderr()).as("Arg: [%s]", arg).isEmpty(); + assertEquals(0, tool.getExitCode()); + assertCorrectEnvPostTest(); + }); } @Test From b47e23f6a47e7c4f99c135b0796e1841dec3d26c Mon Sep 17 00:00:00 2001 From: Arvind Kandpal Date: Wed, 20 May 2026 11:20:07 +0530 Subject: [PATCH 2/2] use constant in addOptions, lower log to debug, and add comment for the while loop --- .../org/apache/cassandra/db/ColumnFamilyStore.java | 12 +++++++++++- .../apache/cassandra/tools/StandaloneUpgrader.java | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index 7a6181401f63..6eb396c95229 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -1026,6 +1026,16 @@ public Descriptor newSSTableDescriptor(File directory, SSTableFormat forma public Descriptor newSSTableDescriptor(File directory, Version version) { Descriptor newDescriptor; + + /* + * Note that thread safety is managed by the underlying sstableIdGenerator (e.g., via AtomicInteger), + * guaranteeing unique IDs for concurrent callers within the process. + * * This loop acts as a collision-resolution mechanism for offline vs. online operations. + * Offline tools (such as bin/sstableupgrade) may generate new SSTables on disk, bypassing + * the in-memory ID generator. If the live process encounters a collision due to such + * out-of-band generation, this loop ensures the process advances the generator until a + * free ID is found. + */ while (true) { newDescriptor = new Descriptor(version, @@ -1037,7 +1047,7 @@ public Descriptor newSSTableDescriptor(File directory, Version version) if (!newDescriptor.fileFor(Components.DATA).exists()) break; - logger.warn("Generated SSTable id {} collides with existing file; advancing.", newDescriptor.id); + logger.debug("Generated SSTable id {} collides with existing file; advancing.", newDescriptor.id); } return newDescriptor; diff --git a/src/java/org/apache/cassandra/tools/StandaloneUpgrader.java b/src/java/org/apache/cassandra/tools/StandaloneUpgrader.java index d8fc0b459a62..3b0e0bcdde00 100644 --- a/src/java/org/apache/cassandra/tools/StandaloneUpgrader.java +++ b/src/java/org/apache/cassandra/tools/StandaloneUpgrader.java @@ -217,7 +217,7 @@ private static CmdLineOptions getCmdLineOptions() options.addOption(null, DEBUG_OPTION, "display stack traces"); options.addOption("h", HELP_OPTION, "display this help message"); options.addOption("k", KEEP_SOURCE, "do not delete the source sstables"); - options.addOption("a", "include-all-sstables", "include all sstables, even those already on the current version"); + options.addOption("a", INCLUDE_ALL, "include all sstables, even those already on the current version"); return options; }