frankgh commented on code in PR #3374:
URL: https://github.com/apache/cassandra/pull/3374#discussion_r1876570565
##########
test/unit/org/apache/cassandra/index/sasi/SASIIndexTest.java:
##########
@@ -199,7 +200,7 @@ public void testSASIComponentsAddedToSnapshot() throws
Throwable
try
{
- store.snapshot(snapshotName);
+ SnapshotManager.instance.takeSnapshot(snapshotName,
store.getKeyspaceTableName());
Review Comment:
not that it matters much, but the equivalent of this is actually:
```suggestion
SnapshotManager.instance.takeSnapshot(snapshotName,
store.getKeyspaceTableName(), store.getTableName());
```
and in fact you clear the snapshot after the test for the specific table
##########
src/java/org/apache/cassandra/db/compaction/CompactionTask.java:
##########
@@ -126,8 +127,8 @@ protected void runMayThrow() throws Exception
if (DatabaseDescriptor.isSnapshotBeforeCompaction())
{
- Instant creationTime = now();
- cfs.snapshotWithoutMemtable(creationTime.toEpochMilli() +
"-compact-" + cfs.name, creationTime);
+ SnapshotOptions options = SnapshotOptions.systemSnapshot(cfs.name,
SnapshotType.COMPACT, cfs.getKeyspaceTableName()).skipFlush().build();
Review Comment:
I don't think we were specifying a single ks/table previously. So
essentially we will only be taking a snapshot of the keyspace/table during the
compaction task. Before we used to take a snapshot of all keyspaces and tables.
I think that's fine for the compaction task, but want to make sure you are
aware of this change and make sure this is intended.
##########
src/java/org/apache/cassandra/db/repair/CassandraValidationIterator.java:
##########
@@ -270,7 +272,7 @@ public void close()
{
// we can only clear the snapshot if we are not doing a global
snapshot validation (we then clear it once anticompaction
// is done).
- cfs.clearSnapshot(snapshotName);
+ SnapshotManager.instance.clearSnapshot(cfs.keyspace.getName(),
cfs.name, snapshotName);
Review Comment:
for consistency with the other changes in this file:
```suggestion
SnapshotManager.instance.clearSnapshot(cfs.getKeyspaceName(),
cfs.getTableName(), snapshotName);
```
##########
src/java/org/apache/cassandra/service/snapshot/TableSnapshot.java:
##########
@@ -287,66 +473,79 @@ protected static String buildSnapshotId(String
keyspaceName, String tableName, U
return String.format("%s:%s:%s:%s", keyspaceName, tableName, tableId,
tag);
}
- public static class SnapshotTrueSizeCalculator extends
DirectorySizeCalculator
+ public static Set<Descriptor> getSnapshotDescriptors(String keyspace,
String table, String tag)
{
- /**
- * Snapshots are composed of hard-linked sstables. The true snapshot
size should only include
- * snapshot files which do not contain a corresponding "live" sstable
file.
- */
- @Override
- public boolean isAcceptable(Path snapshotFilePath)
+ try
{
- return !getLiveFileFromSnapshotFile(snapshotFilePath).exists();
- }
- }
+ Refs<SSTableReader> snapshotSSTableReaders =
getSnapshotSSTableReaders(keyspace, table, tag);
- /**
- * Returns the corresponding live file for a given snapshot file.
- *
- * Example:
- * - Base table:
- * - Snapshot file:
~/.ccm/test/node1/data0/test_ks/tbl-e03faca0813211eca100c705ea09b5ef/snapshots/1643481737850/me-1-big-Data.db
- * - Live file:
~/.ccm/test/node1/data0/test_ks/tbl-e03faca0813211eca100c705ea09b5ef/me-1-big-Data.db
- * - Secondary index:
- * - Snapshot file:
~/.ccm/test/node1/data0/test_ks/tbl-e03faca0813211eca100c705ea09b5ef/snapshots/1643481737850/.tbl_val_idx/me-1-big-Summary.db
- * - Live file:
~/.ccm/test/node1/data0/test_ks/tbl-e03faca0813211eca100c705ea09b5ef/.tbl_val_idx/me-1-big-Summary.db
- *
- */
- static File getLiveFileFromSnapshotFile(Path snapshotFilePath)
- {
- // Snapshot directory structure format is
{data_dir}/snapshots/{snapshot_name}/{snapshot_file}
- Path liveDir = snapshotFilePath.getParent().getParent().getParent();
- if (Directories.isSecondaryIndexFolder(snapshotFilePath.getParent()))
+ Set<Descriptor> descriptors = new HashSet<>();
+ for (SSTableReader ssTableReader : snapshotSSTableReaders)
+ {
+ descriptors.add(ssTableReader.descriptor);
+ }
+
+ return descriptors;
+ }
+ catch (IOException e)
{
- // Snapshot file structure format is
{data_dir}/snapshots/{snapshot_name}/.{index}/{sstable-component}.db
- liveDir = File.getPath(liveDir.getParent().toString(),
snapshotFilePath.getParent().getFileName().toString());
+ throw Throwables.unchecked(e);
}
- return new File(liveDir.toString(),
snapshotFilePath.getFileName().toString());
}
- public static Predicate<TableSnapshot> shouldClearSnapshot(String tag,
long olderThanTimestamp)
+ public static Refs<SSTableReader> getSnapshotSSTableReaders(String
keyspace, String table, String tag) throws IOException
Review Comment:
I'd like to have a signature here that takes a CFS, any reason we don't have
it? it can simplify the call sites
```suggestion
public static Refs<SSTableReader> getSnapshotSSTableReaders(String
keyspace, String table, String tag) throws IOException
{
return
getSnapshotSSTableReaders(Keyspace.open(keyspace).getColumnFamilyStore(table),
tag);
}
public static Refs<SSTableReader>
getSnapshotSSTableReaders(ColumnFamilyStore cfs, String tag)
```
--
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]
