From e6ffd8b4f7ef194446ba0a968e463ca0cd710867 Mon Sep 17 00:00:00 2001 From: Claude Warren Date: Wed, 6 Dec 2023 12:36:43 +0100 Subject: [PATCH] UCS min_sstable_size should not be lower than target_sstable_size lower bound patch by Claude Warren; reviewed by Ethan Brown, Branimir Lambov and Stefan Miklosovic for CASSANDRA-19112 --- CHANGES.txt | 1 + .../db/compaction/unified/Controller.java | 14 +++++-- .../db/compaction/unified/ControllerTest.java | 42 +++++++++++++++++-- 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 3ea5a79229..9405233365 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 5.0-beta2 + * UCS min_sstable_size should not be lower than target_sstable_size lower bound (CASSANDRA-19112) * Fix the correspondingMessagingVersion of SSTable format and improve TTL overflow tests coverage (CASSANDRA-19197) * Fix resource cleanup after SAI query timeouts (CASSANDRA-19177) * Suppress CVE-2023-6481 (CASSANDRA-19184) diff --git a/src/java/org/apache/cassandra/db/compaction/unified/Controller.java b/src/java/org/apache/cassandra/db/compaction/unified/Controller.java index ae9dd4794f..8c3351f038 100644 --- a/src/java/org/apache/cassandra/db/compaction/unified/Controller.java +++ b/src/java/org/apache/cassandra/db/compaction/unified/Controller.java @@ -487,12 +487,14 @@ public class Controller } } + // preserve the configuration for later use during min_sstable_size. + long targetSSTableSize = DEFAULT_TARGET_SSTABLE_SIZE; s = options.remove(TARGET_SSTABLE_SIZE_OPTION); if (s != null) { try { - long targetSSTableSize = FBUtilities.parseHumanReadableBytes(s); + targetSSTableSize = FBUtilities.parseHumanReadableBytes(s); if (targetSSTableSize < MIN_TARGET_SSTABLE_SIZE) { throw new ConfigurationException(String.format("%s %s is not acceptable, size must be at least %s", @@ -596,10 +598,16 @@ public class Controller try { long sizeInBytes = FBUtilities.parseHumanReadableBytes(s); + // zero is a valid option to disable feature if (sizeInBytes < 0) - throw new ConfigurationException(String.format("Invalid configuration, %s should be positive: %s", + throw new ConfigurationException(String.format("Invalid configuration, %s should be greater than or equal to 0 (zero)", + MIN_SSTABLE_SIZE_OPTION)); + int limit = (int) Math.ceil(targetSSTableSize * INVERSE_SQRT_2); + if (sizeInBytes >= limit) + throw new ConfigurationException(String.format("Invalid configuration, %s (%s) should be less than the target size minimum: %s", MIN_SSTABLE_SIZE_OPTION, - s)); + FBUtilities.prettyPrintMemory(sizeInBytes), + FBUtilities.prettyPrintMemory(limit))); } catch (NumberFormatException e) { diff --git a/test/unit/org/apache/cassandra/db/compaction/unified/ControllerTest.java b/test/unit/org/apache/cassandra/db/compaction/unified/ControllerTest.java index 628a15e133..598dae6e6a 100644 --- a/test/unit/org/apache/cassandra/db/compaction/unified/ControllerTest.java +++ b/test/unit/org/apache/cassandra/db/compaction/unified/ControllerTest.java @@ -43,6 +43,8 @@ import org.apache.cassandra.utils.Overlaps; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import static java.lang.String.format; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; @@ -121,8 +123,6 @@ public class ControllerTest { Map options = new HashMap<>(); addOptions(useIntegers, options); - options.putIfAbsent(Controller.MIN_SSTABLE_SIZE_OPTION, FBUtilities.prettyPrintMemory(100 << 20)); - options.putIfAbsent(Controller.SSTABLE_GROWTH_OPTION, "0.5"); options = Controller.validateOptions(options); assertTrue(options.toString(), options.isEmpty()); } @@ -139,8 +139,9 @@ public class ControllerTest options.putIfAbsent(Controller.BASE_SHARD_COUNT_OPTION, Integer.toString(2)); options.putIfAbsent(Controller.TARGET_SSTABLE_SIZE_OPTION, FBUtilities.prettyPrintMemory(100 << 20)); + // The below value is based on the value in the above statement. Decreasing the above statement should result in a decrease below. + options.putIfAbsent(Controller.MIN_SSTABLE_SIZE_OPTION, "70.710MiB"); options.putIfAbsent(Controller.OVERLAP_INCLUSION_METHOD_OPTION, Overlaps.InclusionMethod.SINGLE.toString().toLowerCase()); - options.putIfAbsent(Controller.MIN_SSTABLE_SIZE_OPTION, FBUtilities.prettyPrintMemory(100 << 20)); options.putIfAbsent(Controller.SSTABLE_GROWTH_OPTION, "0.5"); } @@ -553,4 +554,39 @@ public class ControllerTest controller = Controller.fromOptions(cfs, options); assertEquals(Controller.DEFAULT_BASE_SHARD_COUNT, controller.baseShardCount); } + + @Test + public void testMinSSTableSize() + { + Map options = new HashMap<>(); + + // verify 0 is acceptable + options.put(Controller.MIN_SSTABLE_SIZE_OPTION, format("%sB", 0)); + Controller.validateOptions(options); + + // test min < 0 failes + options.put(Controller.MIN_SSTABLE_SIZE_OPTION, "-1B"); + assertThatExceptionOfType(ConfigurationException.class) + .describedAs("Should have thrown a ConfigurationException when min_sstable_size is less than 0") + .isThrownBy(() -> Controller.validateOptions(options)) + .withMessageContaining("greater than or equal to 0"); + + // test min < default target sstable size * INV_SQRT_2 + int limit = (int) Math.ceil(Controller.DEFAULT_TARGET_SSTABLE_SIZE * Controller.INVERSE_SQRT_2); + options.put(Controller.MIN_SSTABLE_SIZE_OPTION, format("%sB", limit + 1)); + assertThatExceptionOfType(ConfigurationException.class) + .describedAs("Should have thrown a ConfigurationException when min_sstable_size is greater than target_sstable_size") + .isThrownBy(() -> Controller.validateOptions(options)) + .withMessageContaining(format("less than the target size minimum: %s", FBUtilities.prettyPrintMemory(limit))); + + // test min < configured target table size * INV_SQRT_2 + limit = (int) Math.ceil(Controller.MIN_TARGET_SSTABLE_SIZE * 2 * Controller.INVERSE_SQRT_2); + options.put(Controller.MIN_SSTABLE_SIZE_OPTION, format("%sB", limit + 1)); + options.put(Controller.TARGET_SSTABLE_SIZE_OPTION, format("%sB", Controller.MIN_TARGET_SSTABLE_SIZE * 2)); + + assertThatExceptionOfType(ConfigurationException.class) + .describedAs("Should have thrown a ConfigurationException when min_sstable_size is greater than target_sstable_size") + .isThrownBy(() -> Controller.validateOptions(options)) + .withMessageContaining(format("less than the target size minimum: %s", FBUtilities.prettyPrintMemory(limit))); + } } \ No newline at end of file