Fix possible NPE on upgrade to 3.0/3.X in case of IO errors patch by Stefania Alborghetti; reviewed by Alex Petrov for CASSANDRA-13389
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/849f8cd6 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/849f8cd6 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/849f8cd6 Branch: refs/heads/cassandra-3.11 Commit: 849f8cd6162c4850d64581a2c4a542c677e43e0a Parents: 451fe9d Author: Stefania Alborghetti <[email protected]> Authored: Wed Mar 29 10:29:26 2017 +0800 Committer: Stefania Alborghetti <[email protected]> Committed: Thu Mar 30 09:05:34 2017 +0800 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../org/apache/cassandra/db/SystemKeyspace.java | 44 ++++---- .../org/apache/cassandra/io/util/FileUtils.java | 26 +++++ .../apache/cassandra/db/SystemKeyspaceTest.java | 106 ++++++++++++++++++- 4 files changed, 154 insertions(+), 23 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/849f8cd6/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index c4293de..5d7b267 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 3.0.13 + * Fix possible NPE on upgrade to 3.0/3.X in case of IO errors (CASSANDRA-13389) * Legacy deserializer can create empty range tombstones (CASSANDRA-13341) * Use the Kernel32 library to retrieve the PID on Windows and fix startup checks (CASSANDRA-13333) * Fix code to not exchange schema across major versions (CASSANDRA-13274) http://git-wip-us.apache.org/repos/asf/cassandra/blob/849f8cd6/src/java/org/apache/cassandra/db/SystemKeyspace.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/SystemKeyspace.java b/src/java/org/apache/cassandra/db/SystemKeyspace.java index da96b38..cc21435 100644 --- a/src/java/org/apache/cassandra/db/SystemKeyspace.java +++ b/src/java/org/apache/cassandra/db/SystemKeyspace.java @@ -64,6 +64,7 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static org.apache.cassandra.cql3.QueryProcessor.executeInternal; import static org.apache.cassandra.cql3.QueryProcessor.executeOnceInternal; +import static org.apache.cassandra.io.util.FileUtils.visitDirectory; public final class SystemKeyspace { @@ -1338,28 +1339,33 @@ public final class SystemKeyspace Iterable<String> dirs = Arrays.asList(DatabaseDescriptor.getAllDataFileLocations()); for (String dataDir : dirs) { - logger.trace("Checking {} for old files", dataDir); + logger.debug("Checking {} for legacy files", dataDir); File dir = new File(dataDir); assert dir.exists() : dir + " should have been created by startup checks"; - for (File ksdir : dir.listFiles((d, n) -> new File(d, n).isDirectory())) - { - logger.trace("Checking {} for old files", ksdir); - - for (File cfdir : ksdir.listFiles((d, n) -> new File(d, n).isDirectory())) - { - logger.trace("Checking {} for old files", cfdir); - - if (Descriptor.isLegacyFile(cfdir)) - { - FileUtils.deleteRecursive(cfdir); - } - else - { - FileUtils.delete(cfdir.listFiles((d, n) -> Descriptor.isLegacyFile(new File(d, n)))); - } - } - } + visitDirectory(dir.toPath(), + File::isDirectory, + ksdir -> + { + logger.trace("Checking {} for legacy files", ksdir); + visitDirectory(ksdir.toPath(), + File::isDirectory, + cfdir -> + { + logger.trace("Checking {} for legacy files", cfdir); + + if (Descriptor.isLegacyFile(cfdir)) + { + FileUtils.deleteRecursive(cfdir); + } + else + { + visitDirectory(cfdir.toPath(), + Descriptor::isLegacyFile, + FileUtils::delete); + } + }); + }); } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/849f8cd6/src/java/org/apache/cassandra/io/util/FileUtils.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/io/util/FileUtils.java b/src/java/org/apache/cassandra/io/util/FileUtils.java index 9e81da5..0bfbbb1 100644 --- a/src/java/org/apache/cassandra/io/util/FileUtils.java +++ b/src/java/org/apache/cassandra/io/util/FileUtils.java @@ -28,6 +28,9 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; +import java.util.function.Predicate; +import java.util.stream.StreamSupport; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -369,6 +372,13 @@ public final class FileUtils public static void delete(File... files) { + if (files == null) + { + // CASSANDRA-13389: some callers use Files.listFiles() which, on error, silently returns null + logger.debug("Received null list of files to delete"); + return; + } + for ( File file : files ) { file.delete(); @@ -387,6 +397,22 @@ public final class FileUtils ScheduledExecutors.nonPeriodicTasks.execute(runnable); } + public static void visitDirectory(Path dir, Predicate<? super File> filter, Consumer<? super File> consumer) + { + try (DirectoryStream<Path> stream = Files.newDirectoryStream(dir)) + { + StreamSupport.stream(stream.spliterator(), false) + .map(Path::toFile) + // stream directories are weakly consistent so we always check if the file still exists + .filter(f -> f.exists() && (filter == null || filter.test(f))) + .forEach(consumer); + } + catch (IOException|DirectoryIteratorException ex) + { + logger.error("Failed to list files in {} with exception: {}", dir, ex.getMessage(), ex); + } + } + public static String stringifyFileSize(double value) { double d; http://git-wip-us.apache.org/repos/asf/cassandra/blob/849f8cd6/test/unit/org/apache/cassandra/db/SystemKeyspaceTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/db/SystemKeyspaceTest.java b/test/unit/org/apache/cassandra/db/SystemKeyspaceTest.java index 3b02979..bcbabfd 100644 --- a/test/unit/org/apache/cassandra/db/SystemKeyspaceTest.java +++ b/test/unit/org/apache/cassandra/db/SystemKeyspaceTest.java @@ -39,12 +39,15 @@ import org.apache.cassandra.utils.ByteBufferUtil; import org.apache.cassandra.utils.FBUtilities; import org.apache.cassandra.utils.CassandraVersion; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.*; public class SystemKeyspaceTest { - public static final String MIGRATION_SSTABLES_ROOT = "migration-sstable-root"; + private static final String MIGRATION_SSTABLES_ROOT = "migration-sstable-root"; + + // any file name will do but unrelated files in our folders tend to be log files or very old data files + private static final String UNRELATED_FILE_NAME = "system.log"; + private static final String UNRELATED_FOLDER_NAME = "snapshot-abc"; @BeforeClass public static void prepSnapshotTracker() @@ -221,7 +224,8 @@ public class SystemKeyspaceTest else { File[] legacyFiles = cfdir.listFiles((d, n) -> Descriptor.isLegacyFile(new File(d, n))); - ret += legacyFiles.length; + if (legacyFiles != null) + ret += legacyFiles.length; } } } @@ -229,6 +233,100 @@ public class SystemKeyspaceTest return ret; } + @Test + public void testMigrateDataDirs_UnrelatedFiles_2_1() throws IOException + { + testMigrateDataDirsWithUnrelatedFiles("2.1"); + } + + @Test + public void testMigrateDataDirs_UnrelatedFiles_2_2() throws IOException + { + testMigrateDataDirsWithUnrelatedFiles("2.2"); + } + + private void testMigrateDataDirsWithUnrelatedFiles(String version) throws IOException + { + Path migrationSSTableRoot = Paths.get(System.getProperty(MIGRATION_SSTABLES_ROOT), version); + Path dataDir = Paths.get(DatabaseDescriptor.getAllDataFileLocations()[0]); + + FileUtils.copyDirectory(migrationSSTableRoot.toFile(), dataDir.toFile()); + + addUnRelatedFiles(dataDir); + + SystemKeyspace.migrateDataDirs(); + + checkUnrelatedFiles(dataDir); + } + + /** + * Add some extra and totally unrelated files to the data dir and its sub-folders + */ + private void addUnRelatedFiles(Path dataDir) throws IOException + { + File dir = new File(dataDir.toString()); + createAndCheck(dir, UNRELATED_FILE_NAME, false); + createAndCheck(dir, UNRELATED_FOLDER_NAME, true); + + for (File ksdir : dir.listFiles((d, n) -> new File(d, n).isDirectory())) + { + createAndCheck(ksdir, UNRELATED_FILE_NAME, false); + createAndCheck(ksdir, UNRELATED_FOLDER_NAME, true); + + for (File cfdir : ksdir.listFiles((d, n) -> new File(d, n).isDirectory())) + { + createAndCheck(cfdir, UNRELATED_FILE_NAME, false); + createAndCheck(cfdir, UNRELATED_FOLDER_NAME, true); + } + } + } + + /** + * Make sure the extra files are still in the data dir and its sub-folders, then + * remove them. + */ + private void checkUnrelatedFiles(Path dataDir) throws IOException + { + File dir = new File(dataDir.toString()); + checkAndDelete(dir, UNRELATED_FILE_NAME, false); + checkAndDelete(dir, UNRELATED_FOLDER_NAME, true); + + for (File ksdir : dir.listFiles((d, n) -> new File(d, n).isDirectory())) + { + checkAndDelete(ksdir, UNRELATED_FILE_NAME, false); + checkAndDelete(ksdir, UNRELATED_FOLDER_NAME, true); + + for (File cfdir : ksdir.listFiles((d, n) -> new File(d, n).isDirectory())) + { + checkAndDelete(cfdir, UNRELATED_FILE_NAME, false); + checkAndDelete(cfdir, UNRELATED_FOLDER_NAME, true); + } + } + } + + private void createAndCheck(File dir, String fileName, boolean isDir) throws IOException + { + File f = new File(dir, fileName); + + if (isDir) + f.mkdir(); + else + f.createNewFile(); + + assertTrue(f.exists()); + } + + private void checkAndDelete(File dir, String fileName, boolean isDir) throws IOException + { + File f = new File(dir, fileName); + assertTrue(f.exists()); + + if (isDir) + FileUtils.deleteDirectory(f); + else + f.delete(); + } + private String getOlderVersionString() { String version = FBUtilities.getReleaseVersionString();
