cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From stefa...@apache.org
Subject [2/6] cassandra git commit: Fix possible NPE on upgrade to 3.0/3.X in case of IO errors
Date Thu, 30 Mar 2017 01:15:19 GMT
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 <stefania.alborghetti@datastax.com>
Authored: Wed Mar 29 10:29:26 2017 +0800
Committer: Stefania Alborghetti <stefania.alborghetti@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();


Mime
View raw message