You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by st...@apache.org on 2017/03/30 01:15:20 UTC
[3/6] cassandra git commit: Fix possible NPE on upgrade to 3.0/3.X in
case of IO errors
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/trunk
Commit: 849f8cd6162c4850d64581a2c4a542c677e43e0a
Parents: 451fe9d
Author: Stefania Alborghetti <st...@datastax.com>
Authored: Wed Mar 29 10:29:26 2017 +0800
Committer: Stefania Alborghetti <st...@datastax.com>
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();