diff --git a/CHANGES.txt b/CHANGES.txt index aa356a2da163..aaa621e052ed 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 7.0 + * Keep SSTables alive while linking for incremental backup and snapshot, failing instead of silently skipping missing required components (CASSANDRA-16047) * Allow CQLSSTableWriter to specify SSTable id generator to use (CASSANDRA-21012) * Reject LIKE patterns with a wildcard (%) anywhere other than the start or end (CASSANDRA-21068) * Support pluggable default role initialization (CASSANDRA-21546) diff --git a/src/java/org/apache/cassandra/db/lifecycle/Tracker.java b/src/java/org/apache/cassandra/db/lifecycle/Tracker.java index b207d86165eb..79f4eb3689c1 100644 --- a/src/java/org/apache/cassandra/db/lifecycle/Tracker.java +++ b/src/java/org/apache/cassandra/db/lifecycle/Tracker.java @@ -66,6 +66,7 @@ import org.apache.cassandra.utils.Throwables; import org.apache.cassandra.utils.TimeUUID; import org.apache.cassandra.utils.concurrent.OpOrder; +import org.apache.cassandra.utils.concurrent.Ref; import static com.google.common.base.Predicates.and; import static com.google.common.collect.ImmutableSet.copyOf; @@ -487,8 +488,20 @@ public void maybeIncrementallyBackup(final Iterable sstables) for (SSTableReader sstable : sstables) { - File backupsDir = Directories.getBackupsDirectory(sstable.descriptor); - sstable.createLinks(FileUtils.getCanonicalPath(backupsDir)); + // addSSTables publishes readers before backing them up, so compaction may already have + // released one. Skip only readers whose lifecycle has ended, never individual components. + Ref ref = sstable.tryRef(); + if (ref == null) + continue; + try + { + File backupsDir = Directories.getBackupsDirectory(sstable.descriptor); + sstable.createLinks(FileUtils.getCanonicalPath(backupsDir)); + } + finally + { + ref.release(); + } } } diff --git a/src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java b/src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java index ab3171200f17..7f449606a3bd 100644 --- a/src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java +++ b/src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java @@ -19,7 +19,9 @@ import java.io.IOException; +import java.io.UncheckedIOException; import java.lang.ref.WeakReference; +import java.nio.file.NoSuchFileException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -1172,7 +1174,7 @@ public static void createLinks(Descriptor descriptor, Set components, } /** - * Create hardlinks for given set of components + * Create hardlinks for every supplied component. The caller must keep the SSTable alive until this returns. * * @param descriptor descriptor to use * @param components components to create links for @@ -1186,7 +1188,11 @@ public static void createLinks(Descriptor descriptor, Set components, { File sourceFile = descriptor.fileFor(component); if (!sourceFile.exists()) - continue; + { + if (descriptor.getFormat().generatedOnLoadComponents().contains(component)) + continue; + throw new UncheckedIOException(new NoSuchFileException(sourceFile.path())); + } if (null != limiter) limiter.acquire(); File targetLink = new File(snapshotDirectoryPath, sourceFile.name()); diff --git a/test/unit/org/apache/cassandra/io/sstable/SSTableReaderTest.java b/test/unit/org/apache/cassandra/io/sstable/SSTableReaderTest.java index 108fb5c15be5..13470d5109e4 100644 --- a/test/unit/org/apache/cassandra/io/sstable/SSTableReaderTest.java +++ b/test/unit/org/apache/cassandra/io/sstable/SSTableReaderTest.java @@ -18,12 +18,16 @@ package org.apache.cassandra.io.sstable; import java.io.IOException; +import java.io.UncheckedIOException; import java.nio.ByteBuffer; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; @@ -54,6 +58,7 @@ import org.apache.cassandra.cql3.UntypedResultSet; import org.apache.cassandra.db.ColumnFamilyStore; import org.apache.cassandra.db.DecoratedKey; +import org.apache.cassandra.db.Directories; import org.apache.cassandra.db.Keyspace; import org.apache.cassandra.db.ReadCommand; import org.apache.cassandra.db.ReadExecutionController; @@ -63,6 +68,7 @@ import org.apache.cassandra.db.compaction.OperationType; import org.apache.cassandra.db.lifecycle.LifecycleTransaction; import org.apache.cassandra.db.lifecycle.SSTableSet; +import org.apache.cassandra.db.lifecycle.Tracker; import org.apache.cassandra.db.lifecycle.View; import org.apache.cassandra.db.partitions.UnfilteredPartitionIterators; import org.apache.cassandra.db.rows.Row; @@ -85,6 +91,7 @@ import org.apache.cassandra.io.sstable.keycache.KeyCacheSupport; import org.apache.cassandra.io.util.File; import org.apache.cassandra.io.util.FileDataInput; +import org.apache.cassandra.io.util.FileUtils; import org.apache.cassandra.io.util.MmappedRegions; import org.apache.cassandra.io.util.PageAware; import org.apache.cassandra.schema.CachingParams; @@ -99,6 +106,8 @@ import static java.lang.String.format; import static org.apache.cassandra.cql3.QueryProcessor.executeInternal; import static org.apache.cassandra.db.ColumnFamilyStore.FlushReason.UNIT_TESTS; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -1481,4 +1490,90 @@ private > T trackReleaseableRef(Supplier refSuppl refsToRelease.add(ref.selfRef()); return ref; } + + @Test + public void testCreateLinksSkipsRegenerableComponentButFailsOnMissingRequired() throws IOException + { + // This reader is never registered with a tracker; this test owns its self reference. + SSTableReader reader = SSTableUtils.prepare().ks(KEYSPACE1).cf(CF_STANDARD).write(Collections.singleton("key")).iterator().next(); + File directory = reader.descriptor.directory; + Ref ref = reader.selfRef(); + try + { + File destination = new File(Files.createTempDirectory(directory.toPath(), "links-")); + Set components = Sets.newHashSet(reader.components); + + Component regenerable = Components.FILTER; + assertTrue(reader.descriptor.getFormat().generatedOnLoadComponents().contains(regenerable)); + assertTrue(components.contains(regenerable)); + Files.delete(reader.descriptor.fileFor(regenerable).toPath()); + + SSTableReader.createLinks(reader.descriptor, components, destination.path(), null, false); + assertFalse(new File(destination, reader.descriptor.fileFor(regenerable).name()).exists()); + assertTrue(new File(destination, reader.descriptor.fileFor(Components.DATA).name()).exists()); + + Component required = Components.STATS; + assertFalse(reader.descriptor.getFormat().generatedOnLoadComponents().contains(required)); + Files.delete(reader.descriptor.fileFor(required).toPath()); + File destination2 = new File(Files.createTempDirectory(directory.toPath(), "links2-")); + assertThatThrownBy(() -> SSTableReader.createLinks(reader.descriptor, components, destination2.path(), null, false)) + .isInstanceOf(UncheckedIOException.class) + .hasCauseInstanceOf(NoSuchFileException.class); + } + finally + { + ref.release(); + LifecycleTransaction.waitForDeletions(); + FileUtils.deleteRecursive(directory); + } + } + + @Test + public void testIncrementalBackupPinsComponentsUntilLinked() throws IOException + { + SSTableReader reader = SSTableUtils.prepare().ks(KEYSPACE1).cf(CF_STANDARD).write(Collections.singleton("key")).iterator().next(); + ColumnFamilyStore cfs = Mockito.mock(ColumnFamilyStore.class); + Mockito.when(cfs.isTableIncrementalBackupsEnabled()).thenReturn(true); + Tracker tracker = new Tracker(cfs, null, false); + File directory = reader.descriptor.directory; + File backups = new File(directory, Directories.BACKUPS_SUBDIR); + Map contents = new HashMap<>(); + for (Component component : reader.components) + contents.put(component, Files.readAllBytes(reader.descriptor.fileFor(component).toPath())); + // Deletes every component once the reader's refcount reaches zero, like a real obsoletion. + reader.markObsolete(() -> { + for (Component component : reader.components) + { + File source = reader.descriptor.fileFor(component); + if (source.exists()) + source.delete(); + } + }); + try + { + SSTableReader racingReader = Mockito.spy(reader); + Mockito.doAnswer(invocation -> { + // Model compaction releasing its last reference right as backup starts linking. + // Without Tracker's own tryRef() pin, this races the deletion above against createLinks. + reader.selfRef().release(); + LifecycleTransaction.waitForDeletions(); + return invocation.callRealMethod(); + }).when(racingReader).createLinks(Mockito.anyString()); + + tracker.maybeIncrementallyBackup(Collections.singleton(racingReader)); + for (Map.Entry entry : contents.entrySet()) + assertArrayEquals(entry.getValue(), Files.readAllBytes(new File(backups, reader.descriptor.fileFor(entry.getKey()).name()).toPath())); + + LifecycleTransaction.waitForDeletions(); + // Tracker's pin deferred the deletion until linking finished, then released it. + for (Component component : reader.components) + assertFalse(reader.descriptor.fileFor(component).exists()); + } + finally + { + reader.selfRef().ensureReleased(); + LifecycleTransaction.waitForDeletions(); + FileUtils.deleteRecursive(directory); + } + } }