diff --git a/CHANGES.txt b/CHANGES.txt index cf87114f26..fc2d85f035 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 5.0 + * Remove deprecated compaction_tombstone_warning_threshold and compaction_large_partition_warning_threshold from yaml (CASSANDRA-18626) * Enhance nodetool compactionstats with additional metrics (CASSANDRA-18305) * Added support for type VECTOR (CASSANDRA-18504) * Expose bootstrap and decommission state to nodetool info (CASSANDRA-18555) diff --git a/NEWS.txt b/NEWS.txt index 1ecd60ab29..0faea57dfa 100644 --- a/NEWS.txt +++ b/NEWS.txt @@ -218,9 +218,11 @@ Deprecation - The config property `compaction_large_partition_warning_threshold` has been deprecated in favour of the new guardrail for partition size. That guardrail is based on the properties `partition_size_warn_threshold` and `partition_size_fail_threshold`. The warn threshold has a very similar behaviour to the old config property. + The old property is still supported for backward compatibility, but now it is disabled by default. - The config property `compaction_tombstone_warning_threshold` has been deprecated in favour of the new guardrail for partition tombstones. That guardrail is based on the properties `partition_tombstones_warn_threshold` and `partition_tombstones_fail_threshold`. The warn threshold has a very similar behaviour to the old config property. + The old property is still supported for backward compatibility, but now it is disabled by default. 4.1 === diff --git a/conf/cassandra.yaml b/conf/cassandra.yaml index dc884e2df9..b8aa59e2ca 100644 --- a/conf/cassandra.yaml +++ b/conf/cassandra.yaml @@ -1560,14 +1560,6 @@ batch_size_fail_threshold: 50KiB # Log WARN on any batches not of type LOGGED than span across more partitions than this limit unlogged_batch_across_partitions_warn_threshold: 10 -# Log a warning when compacting partitions larger than this value. -# As of Cassandra 5.0, this property is deprecated in favour of partition_size_warn_threshold. -compaction_large_partition_warning_threshold: 100MiB - -# Log a warning when writing more tombstones than this value to a partition. -# As of Cassandra 5.0, this property is deprecated in favour of partition_tombstones_warn_threshold. -compaction_tombstone_warning_threshold: 100000 - # GC Pauses greater than 200 ms will be logged at INFO level # This threshold can be adjusted to minimize logging if necessary # Min unit: ms diff --git a/src/java/org/apache/cassandra/config/Config.java b/src/java/org/apache/cassandra/config/Config.java index 37dce34d89..5651c4683a 100644 --- a/src/java/org/apache/cassandra/config/Config.java +++ b/src/java/org/apache/cassandra/config/Config.java @@ -328,13 +328,8 @@ public class Config public volatile Integer concurrent_compactors; @Replaces(oldName = "compaction_throughput_mb_per_sec", converter = Converters.MEBIBYTES_PER_SECOND_DATA_RATE, deprecated = true) public volatile DataRateSpec.LongBytesPerSecondBound compaction_throughput = new DataRateSpec.LongBytesPerSecondBound("64MiB/s"); - @Deprecated - @Replaces(oldName = "compaction_large_partition_warning_threshold_mb", converter = Converters.MEBIBYTES_DATA_STORAGE_INT, deprecated = true) - public volatile DataStorageSpec.IntMebibytesBound compaction_large_partition_warning_threshold = new DataStorageSpec.IntMebibytesBound("100MiB"); @Replaces(oldName = "min_free_space_per_drive_in_mb", converter = Converters.MEBIBYTES_DATA_STORAGE_INT, deprecated = true) public DataStorageSpec.IntMebibytesBound min_free_space_per_drive = new DataStorageSpec.IntMebibytesBound("50MiB"); - @Deprecated - public volatile Integer compaction_tombstone_warning_threshold = 100000; // fraction of free disk space available for compaction after min free space is subtracted public volatile Double max_space_usable_for_compactions_in_percentage = .95; @@ -875,8 +870,11 @@ public class Config public volatile boolean read_before_write_list_operations_enabled = true; public volatile boolean allow_filtering_enabled = true; public volatile boolean simplestrategy_enabled = true; + @Replaces(oldName = "compaction_large_partition_warning_threshold_mb", converter = Converters.LONG_BYTES_DATASTORAGE_MEBIBYTES_INT, deprecated = true) + @Replaces(oldName = "compaction_large_partition_warning_threshold", converter = Converters.LONG_BYTES_DATASTORAGE_MEBIBYTES_DATASTORAGE, deprecated = true) public volatile DataStorageSpec.LongBytesBound partition_size_warn_threshold = null; public volatile DataStorageSpec.LongBytesBound partition_size_fail_threshold = null; + @Replaces(oldName = "compaction_tombstone_warning_threshold", converter = Converters.IDENTITY, deprecated = true) public volatile long partition_tombstones_warn_threshold = -1; public volatile long partition_tombstones_fail_threshold = -1; public volatile DataStorageSpec.LongBytesBound column_value_size_warn_threshold = null; diff --git a/src/java/org/apache/cassandra/config/Converters.java b/src/java/org/apache/cassandra/config/Converters.java index c898c08d64..4b454e2f93 100644 --- a/src/java/org/apache/cassandra/config/Converters.java +++ b/src/java/org/apache/cassandra/config/Converters.java @@ -94,6 +94,12 @@ public enum Converters BYTES_DATASTORAGE(Integer.class, DataStorageSpec.IntBytesBound.class, DataStorageSpec.IntBytesBound::new, o -> o == null ? null : o.toBytes()), + LONG_BYTES_DATASTORAGE_MEBIBYTES_INT(Integer.class, DataStorageSpec.LongBytesBound.class, + o -> o == null ? null : new DataStorageSpec.LongBytesBound(o, DataStorageSpec.DataStorageUnit.MEBIBYTES), + o -> o == null ? null : o.toMebibytesInt()), + LONG_BYTES_DATASTORAGE_MEBIBYTES_DATASTORAGE(DataStorageSpec.IntMebibytesBound.class, DataStorageSpec.LongBytesBound.class, + o -> o == null ? null : new DataStorageSpec.LongBytesBound(o.toBytesInLong()), + o -> o == null ? null : new DataStorageSpec.IntMebibytesBound(o.toMebibytesInt())), /** * This converter is used to support backward compatibility for parameters where in the past negative number was used as a value * Example: native_transport_max_concurrent_requests_in_bytes_per_ip = -1 and native_transport_max_request_data_in_flight_per_ip = null diff --git a/src/java/org/apache/cassandra/config/DataStorageSpec.java b/src/java/org/apache/cassandra/config/DataStorageSpec.java index f0d3acaa61..b5d4374088 100644 --- a/src/java/org/apache/cassandra/config/DataStorageSpec.java +++ b/src/java/org/apache/cassandra/config/DataStorageSpec.java @@ -207,6 +207,14 @@ public abstract class DataStorageSpec { return unit().toBytes(quantity()); } + + /** + * @return the amount of data storage in mebibytes + */ + public int toMebibytesInt() + { + return Ints.saturatedCast(unit().toMebibytes(quantity())); + } } /** diff --git a/src/java/org/apache/cassandra/config/DatabaseDescriptor.java b/src/java/org/apache/cassandra/config/DatabaseDescriptor.java index 5e4e60939f..ce834dd377 100644 --- a/src/java/org/apache/cassandra/config/DatabaseDescriptor.java +++ b/src/java/org/apache/cassandra/config/DatabaseDescriptor.java @@ -2175,24 +2175,6 @@ public class DatabaseDescriptor conf.compaction_throughput = new DataRateSpec.LongBytesPerSecondBound(value, MEBIBYTES_PER_SECOND); } - @Deprecated - public static long getCompactionLargePartitionWarningThreshold() - { - return conf.compaction_large_partition_warning_threshold.toBytesInLong(); - } - - @Deprecated - public static int getCompactionTombstoneWarningThreshold() - { - return conf.compaction_tombstone_warning_threshold; - } - - @Deprecated - public static void setCompactionTombstoneWarningThreshold(int count) - { - conf.compaction_tombstone_warning_threshold = count; - } - public static int getConcurrentValidations() { return conf.concurrent_validations; diff --git a/src/java/org/apache/cassandra/io/sstable/format/SortedTableWriter.java b/src/java/org/apache/cassandra/io/sstable/format/SortedTableWriter.java index 2deda2a6f7..bd71c4758b 100644 --- a/src/java/org/apache/cassandra/io/sstable/format/SortedTableWriter.java +++ b/src/java/org/apache/cassandra/io/sstable/format/SortedTableWriter.java @@ -27,7 +27,6 @@ import com.google.common.collect.ImmutableSet; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.db.DecoratedKey; import org.apache.cassandra.db.DeletionPurger; import org.apache.cassandra.db.DeletionTime; @@ -216,8 +215,6 @@ public abstract class SortedTableWriter

ex long rowSize = endPosition - partitionWriter.getInitialPosition(); guardPartitionThreshold(Guardrails.partitionSize, key, rowSize); guardPartitionThreshold(Guardrails.partitionTombstones, key, metadataCollector.totalTombstones); - maybeLogLargePartitionWarning(key, rowSize); - maybeLogManyTombstonesWarning(key, metadataCollector.totalTombstones); metadataCollector.addPartitionSizeInBytes(rowSize); metadataCollector.addKey(key.getKey()); metadataCollector.addCellPerPartitionCount(); @@ -340,26 +337,6 @@ public abstract class SortedTableWriter

ex } } - @Deprecated - private void maybeLogLargePartitionWarning(DecoratedKey key, long rowSize) - { - if (rowSize > DatabaseDescriptor.getCompactionLargePartitionWarningThreshold()) - { - String keyString = metadata().partitionKeyType.getString(key.getKey()); - logger.warn("Writing large partition {}/{}:{} ({}) to sstable {}", metadata.keyspace, metadata.name, keyString, FBUtilities.prettyPrintMemory(rowSize), getFilename()); - } - } - - @Deprecated - private void maybeLogManyTombstonesWarning(DecoratedKey key, int tombstoneCount) - { - if (tombstoneCount > DatabaseDescriptor.getCompactionTombstoneWarningThreshold()) - { - String keyString = metadata().partitionKeyType.getString(key.getKey()); - logger.warn("Writing {} tombstones to {}/{}:{} in sstable {}", tombstoneCount, metadata.keyspace, metadata.name, keyString, getFilename()); - } - } - private void guardCollectionSize(DecoratedKey partitionKey, Row row) { if (!Guardrails.collectionSize.enabled() && !Guardrails.itemsPerCollection.enabled()) diff --git a/src/java/org/apache/cassandra/service/StorageService.java b/src/java/org/apache/cassandra/service/StorageService.java index 1773673f61..b562e629f6 100644 --- a/src/java/org/apache/cassandra/service/StorageService.java +++ b/src/java/org/apache/cassandra/service/StorageService.java @@ -121,6 +121,7 @@ import org.apache.cassandra.db.SystemKeyspace; import org.apache.cassandra.db.commitlog.CommitLog; import org.apache.cassandra.db.compaction.CompactionManager; import org.apache.cassandra.db.compaction.OperationType; +import org.apache.cassandra.db.guardrails.Guardrails; import org.apache.cassandra.db.lifecycle.LifecycleTransaction; import org.apache.cassandra.db.virtual.VirtualKeyspaceRegistry; import org.apache.cassandra.dht.BootStrapper; @@ -7002,13 +7003,13 @@ public class StorageService extends NotificationBroadcasterSupport implements IE if (count < 0) throw new IllegalStateException("compaction tombstone warning threshold needs to be >= 0, not "+count); logger.info("Setting compaction_tombstone_warning_threshold to {}", count); - DatabaseDescriptor.setCompactionTombstoneWarningThreshold(count); + Guardrails.instance.setPartitionTombstonesThreshold(count, Guardrails.instance.getPartitionTombstonesFailThreshold()); } @Override public int getCompactionTombstoneWarningThreshold() { - return DatabaseDescriptor.getCompactionTombstoneWarningThreshold(); + return Math.toIntExact(Guardrails.instance.getPartitionTombstonesWarnThreshold()); } public void addSnapshot(TableSnapshot snapshot) { diff --git a/test/distributed/org/apache/cassandra/distributed/test/TombstoneWarningTest.java b/test/distributed/org/apache/cassandra/distributed/test/TombstoneWarningTest.java index 9406432b8d..63ce4f2fe7 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/TombstoneWarningTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/TombstoneWarningTest.java @@ -123,7 +123,8 @@ public class TombstoneWarningTest extends TestBaseImpl { long mark = cluster.get(1).logs().mark(); cluster.get(1).flush(KEYSPACE); - String pattern = ".*Writing (?\\d+) tombstones to distributed_test_keyspace/tbl:(?\\d+).*"; + String pattern = ".*Guardrail partition_tombstones violated: " + + "Partition distributed_test_keyspace\\.tbl:(?\\d+).* has (?\\d+) tombstones.*"; LogResult> res = cluster.get(1).logs().grep(mark, pattern); assertEquals(expectedCount, res.getResult().size()); Pattern p = Pattern.compile(pattern); diff --git a/test/distributed/org/apache/cassandra/distributed/test/guardrails/GuardrailPartitionSizeTest.java b/test/distributed/org/apache/cassandra/distributed/test/guardrails/GuardrailPartitionSizeTest.java index 8dc49e14c9..54c1f61b84 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/guardrails/GuardrailPartitionSizeTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/guardrails/GuardrailPartitionSizeTest.java @@ -53,7 +53,6 @@ public class GuardrailPartitionSizeTest extends GuardrailTester cluster = init(Cluster.build(NUM_NODES) .withConfig(c -> c.set("partition_size_warn_threshold", WARN_THRESHOLD + "B") .set("partition_size_fail_threshold", FAIL_THRESHOLD + "B") - .set("compaction_large_partition_warning_threshold", "999GiB") .set("memtable_heap_space", "512MiB")) // avoids flushes .start()); cluster.disableAutoCompaction(KEYSPACE); diff --git a/test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java b/test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java index f1f395893d..6a83dcd4e6 100644 --- a/test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java +++ b/test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java @@ -330,9 +330,6 @@ public class DatabaseDescriptorTest long maxIntMebibytesAsBytes = (long) maxInt * 1024 * 1024; long maxIntKibibytesAsBytes = (long) maxInt * 1024; - conf.compaction_large_partition_warning_threshold = new DataStorageSpec.IntMebibytesBound(maxInt); - Assert.assertEquals(maxIntMebibytesAsBytes, DatabaseDescriptor.getCompactionLargePartitionWarningThreshold()); - conf.min_free_space_per_drive = new DataStorageSpec.IntMebibytesBound(maxInt); Assert.assertEquals(maxIntMebibytesAsBytes, DatabaseDescriptor.getMinFreeSpacePerDriveInBytes()); diff --git a/test/unit/org/apache/cassandra/config/ParseAndConvertUnitsTest.java b/test/unit/org/apache/cassandra/config/ParseAndConvertUnitsTest.java index a21f235524..af18d57aac 100644 --- a/test/unit/org/apache/cassandra/config/ParseAndConvertUnitsTest.java +++ b/test/unit/org/apache/cassandra/config/ParseAndConvertUnitsTest.java @@ -92,7 +92,6 @@ public class ParseAndConvertUnitsTest assertEquals(new DataStorageSpec.IntKibibytesBound(2), config.column_index_cache_size); assertEquals(new DataStorageSpec.IntKibibytesBound(5), config.batch_size_warn_threshold); assertEquals(new DataStorageSpec.IntKibibytesBound(50), config.batch_size_fail_threshold); - assertEquals(new DataStorageSpec.IntMebibytesBound(100), config.compaction_large_partition_warning_threshold); assertNull(config.commitlog_total_space); assertEquals(new DataStorageSpec.IntMebibytesBound(5), config.commitlog_segment_size); assertNull(config.max_mutation_size); //not set explicitly in the default yaml, check the config; not set there too diff --git a/test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java b/test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java index b46d2ea424..1f7819820c 100644 --- a/test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java +++ b/test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java @@ -355,6 +355,27 @@ public class YamlConfigurationLoaderTest // NEGATIVE_MEBIBYTES_DATA_STORAGE_INT assertThat(from("sstable_preemptive_open_interval_in_mb", "1").sstable_preemptive_open_interval.toMebibytes()).isEqualTo(1); assertThat(from("sstable_preemptive_open_interval_in_mb", -2).sstable_preemptive_open_interval).isNull(); + + // LONG_BYTES_DATASTORAGE_MEBIBYTES_INT + assertThat(from("compaction_large_partition_warning_threshold_mb", "42").partition_size_warn_threshold.toMebibytesInt()).isEqualTo(42); + assertThatThrownBy(() -> from("compaction_large_partition_warning_threshold_mb", -1).partition_size_warn_threshold.toMebibytesInt()) + .hasRootCauseInstanceOf(IllegalArgumentException.class) + .hasRootCauseMessage("Invalid data storage: value must be non-negative"); + + // LONG_BYTES_DATASTORAGE_MEBIBYTES_DATASTORAGE + assertThat(from("compaction_large_partition_warning_threshold", "42MiB").partition_size_warn_threshold.toMebibytesInt()).isEqualTo(42); + assertThat(from("compaction_large_partition_warning_threshold", "42GiB").partition_size_warn_threshold.toMebibytesInt()).isEqualTo(42 * 1024); + assertThatThrownBy(() -> from("compaction_large_partition_warning_threshold", "42B").partition_size_warn_threshold.toBytes()) + .hasRootCauseInstanceOf(IllegalArgumentException.class) + .hasRootCauseMessage("Invalid data storage: 42B Accepted units:[MEBIBYTES, GIBIBYTES]"); + assertThatThrownBy(() -> from("compaction_large_partition_warning_threshold", -1).partition_size_warn_threshold.toMebibytesInt()) + .hasRootCauseInstanceOf(IllegalArgumentException.class) + .hasRootCauseMessage("Invalid data storage: -1 Accepted units:[MEBIBYTES, GIBIBYTES] where case matters and only non-negative values are accepted"); + + // IDENTITY + assertThat(from("compaction_tombstone_warning_threshold", "42").partition_tombstones_warn_threshold).isEqualTo(42); + assertThat(from("compaction_tombstone_warning_threshold", "-1").partition_tombstones_warn_threshold).isEqualTo(-1); + assertThat(from("compaction_tombstone_warning_threshold", "0").partition_tombstones_warn_threshold).isEqualTo(0); } private static Config from(Object... values)