[GitHub] [cassandra] yifan-c commented on a change in pull request #971: CASSANDRA-15669. Fixed ArrayIndexOutOfBoundsException in LCS for L8 compaction.

[GitBox <gi...@apache.org>]

yifan-c commented on a change in pull request #971:
URL: https://github.com/apache/cassandra/pull/971#discussion_r617769682



##########
File path: test/unit/org/apache/cassandra/db/compaction/LeveledCompactionStrategyTest.java
##########
@@ -823,4 +823,21 @@ private static void assertLevelsEqual(Collection<SSTableReader> l1, Collection<S
         assertEquals(l1.size(), l2.size());
         assertEquals(new HashSet<>(l1), new HashSet<>(l2));
     }
+
+    @Test(expected = RuntimeException.class)

Review comment:
       Only excepting a `RuntimeException` is not sufficient. 
   
   `assertThatThrownBy` is a better alternative.
   e.g. https://github.com/apache/cassandra/blob/27cc2fc3e275f56f1fa1df7285c389d5491acc8c/test/unit/org/apache/cassandra/db/marshal/EmptyTypeTest.java#L95-L100

##########
File path: src/java/org/apache/cassandra/db/compaction/LeveledGenerations.java
##########
@@ -49,9 +49,10 @@
 {
     private static final Logger logger = LoggerFactory.getLogger(LeveledGenerations.class);
     private final boolean strictLCSChecksTest = Boolean.getBoolean(Config.PROPERTY_PREFIX + "test.strict_lcs_checks");
-    // allocate enough generations for a PB of data, with a 1-MB sstable size.  (Note that if maxSSTableSize is
-    // updated, we will still have sstables of the older, potentially smaller size.  So don't make this
-    // dependent on maxSSTableSize.)
+    // Allocate enough generations to handle about 95 TB of data, with a 1-MB sstable size and default fanout size
+    // (10). (Note that if maxSSTableSize is updated, we will still have sstables of the older, potentially smaller
+    // size. So don't make this dependent on maxSSTableSize). Max level count actually includes L0 and finally, we
+    // support [L0 - L8] levels with the current implementation.

Review comment:
       How about just setting `MAX_LEVEL_COUNT` to 9, and getting rid of the "promise" of the max allowed data size? 
   The `Math.log` calculation is unnecessary. Changing it to 9 is clear to read. 
   The max data size depends on the unit size and fanout size, and it is likely not matching with the comment. 

##########
File path: src/java/org/apache/cassandra/db/compaction/LeveledManifest.java
##########
@@ -242,11 +242,17 @@ public synchronized CompactionCandidate getCompactionCandidates()
             // we want to calculate score excluding compacting ones
             Set<SSTableReader> sstablesInLevel = Sets.newHashSet(sstables);
             Set<SSTableReader> remaining = Sets.difference(sstablesInLevel, cfs.getTracker().getCompacting());
-            double score = (double) SSTableReader.getTotalBytes(remaining) / (double)maxBytesForLevel(i, maxSSTableSizeInBytes);
+            long remainingBytesForLevel = SSTableReader.getTotalBytes(remaining);
+            long maxBytesForLevel = maxBytesForLevel(i, maxSSTableSizeInBytes);
+            double score = (double) remainingBytesForLevel / (double) maxBytesForLevel;
             logger.trace("Compaction score for level {} is {}", i, score);
 
             if (score > 1.001)
             {
+                // the highest level should not ever exceed its maximum size
+                if (i == generations.levelCount() - 1)
+                    throw new RuntimeException("L" + i + " should not exceed its maximum size (" + maxBytesForLevel + "), but it has " + remainingBytesForLevel + " bytes");

Review comment:
       nit:  add an annotation to indicate `"L" + i` is the max supported level. It helps to reason that why that level cannot compact further.
   ```suggestion
                       throw new RuntimeException("L" + i + " (maximum supported level) should not exceed its maximum size (" + maxBytesForLevel + "), but it has " + remainingBytesForLevel + " bytes");
   Write
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org