Skip to content

CASSANDRA-21133 Make bin/sstableupgrade functionally on par with nodetool upgradesstables #4825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
arvindKandpal-ksolves wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

CASSANDRA-21133 Make bin/sstableupgrade functionally on par with nodetool upgradesstables #4825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8ad005
CASSANDRA-21133: Make bin/sstableupgrade functionally on par with nod…
arvindKandpal-ksolves May 18, 2026
b47e23f
use constant in addOptions, lower log to debug, and add comment for …
arvindKandpal-ksolves May 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/modules/cassandra/pages/managing/tools/sstable/sstableupgrade.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ sstableupgrade <options> <keyspace> <table> [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
|===

Expand Down
31 changes: 25 additions & 6 deletions src/java/org/apache/cassandra/db/ColumnFamilyStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,12 +1025,31 @@ 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;

/*
* 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)
Copy link
Copy Markdown
Contributor

@smiklosovic smiklosovic May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might not happen in practice but it is worth saying that there is no limit as in how many times we loop, if there is some buggy ID supplier or we exhausted the namespace of these ids and all were on disk then we would spin for ever. Currently we have sequence based generator on integer and UUID generator on ... uuid, the namespace is pretty much "infinite" in practical terms so this we will not hit it, one would have to have like ~2 billions sstables on disk to exhaust (and then it would start to overflow)

Copy link
Copy Markdown
Contributor Author

@arvindKandpal-ksolves arvindKandpal-ksolves May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agreed. With ~2 billion for sequence IDs and practically infinite for UUIDs, we shouldn't ever hit an infinite loop in the real case. Thanks for pointing this out.

Copy link
Copy Markdown
Contributor

@smiklosovic smiklosovic May 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that thread safety is just delegated to how sstableIdGenerator.get() returns the new id, right? That is AtomicInteger, hence increased atomically, so two threads calling this stuff will always have different ids to check the existence of a file against.

@arvindKandpal-ksolves could you put a comment on while(true) about this and that this loop is meant to be a defense against offline / online generation for tooling's sake instead of in-process flow?

Copy link
Copy Markdown
Contributor Author

@arvindKandpal-ksolves arvindKandpal-ksolves May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. The atomic increment takes care of thread safety in-process. I've added a comment above the while(true) loop to explain that this is purely a defense against offline tool generation.

{
newDescriptor = new Descriptor(version,
directory,
getKeyspaceName(),
name,
sstableIdGenerator.get());

if (!newDescriptor.fileFor(Components.DATA).exists())
break;

logger.debug("Generated SSTable id {} collides with existing file; advancing.", newDescriptor.id);
}

return newDescriptor;
}

Expand Down
6 changes: 5 additions & 1 deletion src/java/org/apache/cassandra/tools/StandaloneUpgrader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[])
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
}
Expand All @@ -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, "include all sstables, even those already on the current version");
return options;
}

Expand Down
50 changes: 50 additions & 0 deletions test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -839,6 +843,52 @@ public static long getSnapshotManifestAndSchemaFileSizes(TableSnapshot snapshot)
return schemaAndManifestFileSizes;
}

@Test
public void testNewSSTableDescriptorCollision() throws Exception
Copy link
Copy Markdown
Contributor

@smiklosovic smiklosovic May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just saying we are not testing UUID-based generator but I dont think doing that is the absolute must given how it behaves.

Copy link
Copy Markdown
Contributor Author

@arvindKandpal-ksolves arvindKandpal-ksolves May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if needed we can try to add , otherwise I also thought there is no absolute required.

{
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)
Expand Down
18 changes: 15 additions & 3 deletions test/unit/org/apache/cassandra/tools/StandaloneUpgraderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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
Expand Down