fix compaction NPE when out of disk space and assertions disabled

patch by jbellis; reviewed by xedin for CASSANDRA-3985

CHANGES.txt

@@ -12,6 +12,8 @@
  * fix stress tool that hangs forever on timeout or error (CASSANDRA-4128)
  * Fix super columns bug where cache is not updated (CASSANDRA-4190)
  * stress tool to return appropriate exit code on failure (CASSANDRA-4188)
+ * fix compaction NPE when out of disk space and assertions disabled
+   (CASSANDRA-3985)
 
 
 1.0.9
@@ -27,8 +29,6 @@
  * don't change manifest level for cleanup, scrub, and upgradesstables
    operations under LeveledCompactionStrategy (CASSANDRA-3989, 4112)
  * fix race leading to super columns assertion failure (CASSANDRA-3957)
- * ensure that directory is selected for compaction for user-defined
-   tasks and upgradesstables (CASSANDRA-3985)
  * fix NPE on invalid CQL delete command (CASSANDRA-3755)
  * allow custom types in CLI's assume command (CASSANDRA-4081)
  * fix totalBytes count for parallel compactions (CASSANDRA-3758)

src/java/org/apache/cassandra/config/DatabaseDescriptor.java

@@ -757,23 +757,19 @@ public class DatabaseDescriptor
         return Collections.unmodifiableSet(new HashSet(seedProvider.getSeeds()));
     }
 
-    public synchronized static String getDataFileLocationForTable(String table, long expectedCompactedFileSize)
-    {
-        return getDataFileLocationForTable(table, expectedCompactedFileSize, true);
-    }
-
     /*
      * Loop through all the disks to see which disk has the max free space
      * return the disk with max free space for compactions. If the size of the expected
      * compacted file is greater than the max disk space available return null, we cannot
      * do compaction in this case.
      *
+     * Should only be called by Table.getDataFileLocation, which knows how to free up extra space under
+     * some contitions to retry.  (Left public because some test methods cheat and call this directly.)
+     *
      * @param table name of the table.
      * @param expectedCompactedSize expected file size in bytes.
-     * @param ensureFreeSpace Flag if the function should ensure enough free space exists for the expected file size.
-     *                        If False and there is not enough free space a warning is logged, and the dir with the most space is returned.
      */
-    public synchronized static String getDataFileLocationForTable(String table, long expectedCompactedFileSize, boolean ensureFreeSpace)
+    public synchronized static String getDataFileLocationForTable(String table, long expectedCompactedFileSize)
     {
         long maxFreeDisk = 0;
         int maxDiskIndex = 0;
@@ -791,22 +787,13 @@ public class DatabaseDescriptor
             }
         }
 
-        logger.debug("expected data files size is {}; largest free partition has {} bytes free",
-                     expectedCompactedFileSize,
-                     maxFreeDisk);
-
         // Load factor of 0.9 we do not want to use the entire disk that is too risky.
         maxFreeDisk = (long) (0.9 * maxFreeDisk);
-        if (!ensureFreeSpace || expectedCompactedFileSize < maxFreeDisk)
+        logger.debug("expected data files size is {}; largest free partition has {} bytes usable",
-        {
+                     expectedCompactedFileSize, maxFreeDisk);
-            dataFileDirectory = dataDirectoryForTable[maxDiskIndex];
 
-            if (expectedCompactedFileSize >= maxFreeDisk)
+        if (expectedCompactedFileSize < maxFreeDisk)
-                logger.warn(String.format("Data file location %s only has %d free, expected size is %d",
+            dataFileDirectory = dataDirectoryForTable[maxDiskIndex];
-                                          dataFileDirectory,
-                                          maxFreeDisk,
-                                          expectedCompactedFileSize));
-        }
 
         return dataFileDirectory;
     }

src/java/org/apache/cassandra/db/Table.java

@@ -556,17 +556,15 @@ public class Table
 
     public String getDataFileLocation(long expectedSize)
     {
-        return getDataFileLocation(expectedSize, true);
+        String path = DatabaseDescriptor.getDataFileLocationForTable(name, expectedSize);
-    }
 
-    public String getDataFileLocation(long expectedSize, boolean ensureFreeSpace)
-    {
-        String path = DatabaseDescriptor.getDataFileLocationForTable(name, expectedSize, ensureFreeSpace);
         // Requesting GC has a chance to free space only if we're using mmap and a non SUN jvm
         if (path == null
-         && (DatabaseDescriptor.getDiskAccessMode() == Config.DiskAccessMode.mmap || DatabaseDescriptor.getIndexAccessMode() == Config.DiskAccessMode.mmap)
+            && (DatabaseDescriptor.getDiskAccessMode() == Config.DiskAccessMode.mmap || DatabaseDescriptor.getIndexAccessMode() == Config.DiskAccessMode.mmap)
-         && !MmappedSegmentedFile.isCleanerAvailable())
+            && !MmappedSegmentedFile.isCleanerAvailable())
         {
+            logger.info("Forcing GC to free up disk space.  Upgrade to the Oracle JVM to avoid this");
+
             StorageService.instance.requestGC();
             // retry after GCing has forced unmap of compacted SSTables so they can be deleted
             // Note: GCInspector will do this already, but only sun JVM supports GCInspector so far
@@ -579,8 +577,9 @@ public class Table
             {
                 throw new AssertionError(e);
             }
-            path = DatabaseDescriptor.getDataFileLocationForTable(name, expectedSize, ensureFreeSpace);
+            path = DatabaseDescriptor.getDataFileLocationForTable(name, expectedSize);
         }
+
         return path;
     }
 

src/java/org/apache/cassandra/db/compaction/CompactionTask.java

@@ -77,7 +77,7 @@ public class CompactionTask extends AbstractCompactionTask
             return 0;
 
         if (compactionFileLocation == null)
-            compactionFileLocation = cfs.table.getDataFileLocation(cfs.getExpectedCompactedFileSize(toCompact), ensureFreeSpace());
+            compactionFileLocation = cfs.table.getDataFileLocation(cfs.getExpectedCompactedFileSize(toCompact));
 
         if (compactionFileLocation == null && partialCompactionsAcceptable())
         {
@@ -89,17 +89,14 @@ public class CompactionTask extends AbstractCompactionTask
                 // Note that we have removed files that are still marked as compacting.
                 // This suboptimal but ok since the caller will unmark all the sstables at the end.
                 toCompact.remove(cfs.getMaxSizeFile(toCompact));
-                compactionFileLocation = cfs.table.getDataFileLocation(cfs.getExpectedCompactedFileSize(toCompact),
+                compactionFileLocation = cfs.table.getDataFileLocation(cfs.getExpectedCompactedFileSize(toCompact));
-                                                                       ensureFreeSpace());
-            }
-
-            if (compactionFileLocation == null)
-            {
-                logger.warn("insufficient space to compact even the two smallest files, aborting");
-                return 0;
             }
         }
-        assert compactionFileLocation != null;
+        if (compactionFileLocation == null)
+        {
+            logger.warn("insufficient space to compact; aborting compaction");
+            return 0;
+        }
 
         if (DatabaseDescriptor.isSnapshotBeforeCompaction())
             cfs.snapshotWithoutFlush(System.currentTimeMillis() + "-" + "compact-" + cfs.columnFamily);
@@ -231,11 +228,6 @@ public class CompactionTask extends AbstractCompactionTask
         return !isUserDefined;
     }
 
-    protected boolean ensureFreeSpace()
-    {
-        return !isUserDefined;
-    }
-
     //extensibility point for other strategies that may want to limit the upper bounds of the sstable segment size
     protected boolean newSSTableSegmentThresholdReached(SSTableWriter writer, long position)
     {