Guardrail values of zero to mean literally zero

Before guardrail values of -1 and 0 both mean unlimited. Now zero means zero, for example "more than 0 is a warning" or "more than 0 is a failure".

 patch by Mick Semb Wever; reviewed by David Capwell, Jeremiah Jordan for CASSANDRA-21517

Author: michaelsembwever

CHANGES.txt

@@ -1,4 +1,5 @@
 6.0-alpha3
+ * Guardrail configurations of zero are now treated as zero instead of unlimited (CASSANDRA-21517)
  * Support multi-cell columns in cursor compaction (CASSANDRA-21463)
  * Optimize authorization logic for BatchStatement and TransactionStatement when a single table is updated (CASSANDRA-21606)
  * Upgrade lz4-java to 1.11.2 (CASSANDRA-21564)

NEWS.txt

@@ -209,6 +209,7 @@ Upgrading
     unchanged, and the datacenter and CIDR checks that ride along with them still run at least once per
     request. Auth cache request/hit/miss counts and the system_views.cidr_filtering_metrics_* counts and
     latencies will therefore report fewer events than before. (see CASSANDRA-21606)
+    - Guardrail configurations of zero are now treated as zero instead of unlimited. See CASSANDRA-21517.
 
 Deprecation
 -----------

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

@@ -57,14 +57,14 @@ protected boolean compare(long value, long threshold)
     public long failValue(ClientState state)
     {
         long failValue = failThreshold.applyAsLong(state);
-        return failValue <= 0 ? Long.MAX_VALUE : failValue;
+        return failValue < 0 ? Long.MAX_VALUE : failValue;
     }
 
     @Override
     public long warnValue(ClientState state)
     {
         long warnValue = warnThreshold.applyAsLong(state);
-        return warnValue <= 0 ? Long.MAX_VALUE : warnValue;
+        return warnValue < 0 ? Long.MAX_VALUE : warnValue;
     }
 
 }

src/java/org/apache/cassandra/db/guardrails/MaxThreshold.java

@@ -57,14 +57,14 @@ protected boolean compare(long value, long threshold)
     protected long failValue(ClientState state)
     {
         long failValue = failThreshold.applyAsLong(state);
-        return failValue <= 0 ? Long.MIN_VALUE : failValue;
+        return failValue < 0 ? Long.MIN_VALUE : failValue;
     }
 
     @Override
     protected long warnValue(ClientState state)
     {
         long warnValue = warnThreshold.applyAsLong(state);
-        return warnValue <= 0 ? Long.MIN_VALUE : warnValue;
+        return warnValue < 0 ? Long.MIN_VALUE : warnValue;
     }
 
 }

src/java/org/apache/cassandra/db/guardrails/MinThreshold.java

@@ -83,7 +83,9 @@ public boolean enabled(@Nullable ClientState state)
         if (!super.enabled(state))
             return false;
 
-        return failThreshold.applyAsLong(state) > 0 || warnThreshold.applyAsLong(state) > 0;
+        // A negative threshold is the disabled marker. Zero is a real threshold, meaning "any value above zero
+        // triggers", so a guardrail with either threshold set to zero is enabled.
+        return failThreshold.applyAsLong(state) >= 0 || warnThreshold.applyAsLong(state) >= 0;
     }
 
     /**

src/java/org/apache/cassandra/db/guardrails/Threshold.java

@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.config;
+
+import org.assertj.core.api.Assertions;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+
+/**
+ * Tests the guardrail thresholds a {@code cassandra.yaml} can carry, which {@link GuardrailsOptions} validates on
+ * construction. See CASSANDRA-21517 for the treatment of zero.
+ */
+public class GuardrailsOptionsTest
+{
+    @BeforeClass
+    public static void setupDatabaseDescriptor()
+    {
+        DatabaseDescriptor.daemonInitialization();
+    }
+
+    @Test
+    public void testZeroThresholdsAreAccepted()
+    {
+        Config config = new Config();
+        config.tables_fail_threshold = 0;
+        config.keyspaces_warn_threshold = 0;
+        config.partition_tombstones_fail_threshold = 0;
+        config.data_disk_usage_percentage_warn_threshold = 0;
+        config.partition_size_fail_threshold = new DataStorageSpec.LongBytesBound("0B");
+        config.column_value_size_warn_threshold = new DataStorageSpec.LongBytesBound("0B");
+
+        GuardrailsOptions options = new GuardrailsOptions(config);
+
+        assertEquals(0, options.getTablesFailThreshold());
+        assertEquals(0, options.getKeyspacesWarnThreshold());
+        assertEquals(0, options.getPartitionTombstonesFailThreshold());
+        assertEquals(0, options.getDataDiskUsagePercentageWarnThreshold());
+        assertEquals(0, options.getPartitionSizeFailThreshold().toBytes());
+        assertEquals(0, options.getColumnValueSizeWarnThreshold().toBytes());
+    }
+
+    @Test
+    public void testMinusOneAndNullDisableThresholds()
+    {
+        Config config = new Config();
+        config.tables_fail_threshold = -1;
+        config.partition_size_fail_threshold = null;
+
+        GuardrailsOptions options = new GuardrailsOptions(config);
+
+        assertEquals(-1, options.getTablesFailThreshold());
+        assertNull(options.getPartitionSizeFailThreshold());
+    }
+
+    @Test
+    public void testValuesBelowMinusOneAreRejected()
+    {
+        Config config = new Config();
+        config.tables_fail_threshold = -2;
+
+        Assertions.assertThatThrownBy(() -> new GuardrailsOptions(config))
+                  .isInstanceOf(IllegalArgumentException.class)
+                  .hasMessageContaining("negative values are not allowed, outside of -1 which disables the guardrail");
+    }
+}

test/unit/org/apache/cassandra/config/GuardrailsOptionsTest.java

@@ -126,8 +126,9 @@ public void testConfigValidation()
         // We want to test with very big number, Long.MAX_VALUE is not allowed so it was easy to use Intger.MAX_VALUE
         assertConfigFails(x -> x.setDataDiskUsageMaxDiskSize(Integer.MAX_VALUE + "GiB"), message);
 
-        // warn threshold smaller than lower bound
-        assertConfigFails(x -> x.setDataDiskUsagePercentageThreshold(0, 80), "0 is not allowed");
+        // a warn threshold of zero warns on any disk usage above zero, it does not disable the guardrail
+        assertConfigValid(x -> x.setDataDiskUsagePercentageThreshold(0, 80));
+        assertEquals(0, guardrails().getDataDiskUsagePercentageWarnThreshold());
 
         // fail threshold bigger than upper bound
         assertConfigFails(x -> x.setDataDiskUsagePercentageThreshold(1, 110), "maximum allowed value is 100");

test/unit/org/apache/cassandra/db/guardrails/GuardrailDiskUsageTest.java

@@ -51,6 +51,14 @@ public GuardrailMaximumReplicationFactorTest()
               Guardrails::getMaximumReplicationFactorFailThreshold);
     }
 
+    @Override
+    protected boolean zeroIsValid()
+    {
+        // a maximum replication factor of zero would be lower than default_keyspace_rf, which is rejected
+        // by validateMaxRFThreshold rather than by the threshold validation itself
+        return false;
+    }
+
     @After
     public void cleanupTest() throws Throwable
     {

test/unit/org/apache/cassandra/db/guardrails/GuardrailMaximumReplicationFactorTest.java

@@ -106,14 +106,14 @@ public void testConfigValidation()
         assertInvalidPositiveIntProperty((g, a) -> setter.accept(g, DISABLED_GUARDRAIL, a), Integer.MIN_VALUE, Integer.MAX_VALUE, WHAT + "_fail_threshold");
         assertInvalidPositiveIntProperty((g, a) -> setter.accept(g, DISABLED_GUARDRAIL, a), -2, Integer.MAX_VALUE, WHAT + "_fail_threshold");
         assertValidProperty((g, a) -> setter.accept(g, DISABLED_GUARDRAIL, a), DISABLED_GUARDRAIL);
-        assertInvalidPositiveIntProperty((g, a) -> setter.accept(g, DISABLED_GUARDRAIL, a), 0, Integer.MAX_VALUE, WHAT + "_fail_threshold");
+        assertValidProperty((g, a) -> setter.accept(g, DISABLED_GUARDRAIL, a), 0);
         assertValidProperty((g, a) -> setter.accept(g, DISABLED_GUARDRAIL, a), 1);
         assertValidProperty((g, a) -> setter.accept(g, DISABLED_GUARDRAIL, a), 2);
 
         assertInvalidPositiveIntProperty((g, w) -> setter.accept(g, w, DISABLED_GUARDRAIL), Integer.MIN_VALUE, Integer.MAX_VALUE, WHAT + "_warn_threshold");
         assertInvalidPositiveIntProperty((g, w) -> setter.accept(g, w, DISABLED_GUARDRAIL), -2, Integer.MAX_VALUE, WHAT + "_warn_threshold");
         assertValidProperty((g, w) -> setter.accept(g, w, DISABLED_GUARDRAIL), DISABLED_GUARDRAIL);
-        assertInvalidPositiveIntProperty((g, w) -> setter.accept(g, w, DISABLED_GUARDRAIL), 0, Integer.MAX_VALUE, WHAT + "_warn_threshold");
+        assertValidProperty((g, w) -> setter.accept(g, w, DISABLED_GUARDRAIL), 0);
         assertValidProperty((g, w) -> setter.accept(g, w, DISABLED_GUARDRAIL), 1);
         assertValidProperty((g, w) -> setter.accept(g, w, DISABLED_GUARDRAIL), 2);