Fix leak of non-standard Java types in our Exceptions as clients usin…

…g JMX are unable to handle them

Remove useless validation that leads to unnecessary additional read of cassandra.yaml on startup
patch by Ekaterina Dimitrova; review by David Capwell for CASSANDRA-17638

CHANGES.txt

@@ -1,4 +1,6 @@
 4.1
+ * Fix leak of non-standard Java types in our Exceptions as clients using JMX are unable to handle them.
+   Remove useless validation that leads to unnecessary additional read of cassandra.yaml on startup (CASSANDRA-17638)
  * Fix repair_request_timeout_in_ms and remove paxos_auto_repair_threshold_mb (CASSANDRA-17557)
  * Incremental repair leaks SomeRepairFailedException after switch away from flatMap (CASSANDRA-17620)
  * StorageService read threshold get methods throw NullPointerException due to not handling null configs (CASSANDRA-17593)

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

@@ -25,8 +25,6 @@
 
 import com.google.common.primitives.Ints;
 
-import org.apache.cassandra.exceptions.ConfigurationException;
-
 /**
  * Represents a data rate type used for cassandra configuration. It supports the opportunity for the users to be able to
  * add units to the confiuration parameter value. (CASSANDRA-15234)
@@ -48,7 +46,7 @@ public DataRateSpec(String value)
         Matcher matcher = BIT_RATE_UNITS_PATTERN.matcher(value);
 
         if (!matcher.find())
-            throw new ConfigurationException("Invalid bit rate: " + value + " Accepted units: MiB/s, KiB/s, B/s where " +
+            throw new IllegalArgumentException("Invalid bit rate: " + value + " Accepted units: MiB/s, KiB/s, B/s where " +
                                              "case matters and " + "only non-negative values are valid");
 
         quantity = Long.parseLong(matcher.group(1));
@@ -58,7 +56,7 @@ public DataRateSpec(String value)
     DataRateSpec(double quantity, DataRateUnit unit)
     {
         if (quantity < 0)
-            throw new ConfigurationException("Invalid bit rate: value must be non-negative");
+            throw new IllegalArgumentException("Invalid bit rate: value must be non-negative");
 
         if (quantity > Long.MAX_VALUE)
             throw new NumberFormatException("Invalid bit rate: value must be between 0 and Long.MAX_VALUE = 9223372036854775807");
@@ -337,7 +335,7 @@ public static DataRateUnit fromSymbol(String symbol)
                 if (value.symbol.equalsIgnoreCase(symbol))
                     return value;
             }
-            throw new ConfigurationException(String.format("Unsupported data rate unit: %s. Supported units are: %s",
+            throw new IllegalArgumentException(String.format("Unsupported data rate unit: %s. Supported units are: %s",
                                                              symbol, Arrays.stream(values())
                                                                            .map(u -> u.symbol)
                                                                            .collect(Collectors.joining(", "))));

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

@@ -27,8 +27,6 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.primitives.Ints;
 
-import org.apache.cassandra.exceptions.ConfigurationException;
-
 import static org.apache.cassandra.config.DataStorageSpec.DataStorageUnit.GIBIBYTES;
 import static org.apache.cassandra.config.DataStorageSpec.DataStorageUnit.KIBIBYTES;
 import static org.apache.cassandra.config.DataStorageSpec.DataStorageUnit.MEBIBYTES;
@@ -62,7 +60,7 @@ public DataStorageSpec(String value)
 
         if (!matcher.find())
         {
-            throw new ConfigurationException("Invalid data storage: " + value + " Accepted units: B, KiB, MiB, GiB" +
+            throw new IllegalArgumentException("Invalid data storage: " + value + " Accepted units: B, KiB, MiB, GiB" +
                                              " where case matters and only non-negative values are accepted");
         }
 
@@ -73,7 +71,7 @@ public DataStorageSpec(String value)
     DataStorageSpec(long quantity, DataStorageUnit unit)
     {
         if (quantity < 0)
-            throw new ConfigurationException("Invalid data storage: value must be positive, but was " + quantity);
+            throw new IllegalArgumentException("Invalid data storage: value must be positive, but was " + quantity);
 
         this.quantity = quantity;
         this.unit = unit;
@@ -82,7 +80,7 @@ public DataStorageSpec(String value)
     public DataStorageSpec (String value, DataStorageUnit minUnit)
     {
         if (!MAP_UNITS_PER_MIN_UNIT.containsKey(minUnit))
-            throw new ConfigurationException("Invalid smallest unit set for " + value);
+            throw new IllegalArgumentException("Invalid smallest unit set for " + value);
 
         //parse the string field value
         Matcher matcher = STORAGE_UNITS_PATTERN.matcher(value);
@@ -93,11 +91,11 @@ public DataStorageSpec (String value, DataStorageUnit minUnit)
             unit = DataStorageUnit.fromSymbol(matcher.group(2));
 
             if (!MAP_UNITS_PER_MIN_UNIT.get(minUnit).contains(unit))
-                throw new ConfigurationException("Invalid data storage: " + value + " Accepted units:" + MAP_UNITS_PER_MIN_UNIT);
+                throw new IllegalArgumentException("Invalid data storage: " + value + " Accepted units:" + MAP_UNITS_PER_MIN_UNIT);
         }
         else
         {
-            throw new ConfigurationException("Invalid data storage: " + value + " Accepted units:" + MAP_UNITS_PER_MIN_UNIT.get(minUnit) +
+            throw new IllegalArgumentException("Invalid data storage: " + value + " Accepted units:" + MAP_UNITS_PER_MIN_UNIT.get(minUnit) +
                                              " where case matters and only non-negative values are accepted");
         }
     }
@@ -389,7 +387,7 @@ public static DataStorageUnit fromSymbol(String symbol)
                 if (value.symbol.equalsIgnoreCase(symbol))
                     return value;
             }
-            throw new ConfigurationException(String.format("Unsupported data storage unit: %s. Supported units are: %s",
+            throw new IllegalArgumentException(String.format("Unsupported data storage unit: %s. Supported units are: %s",
                                                            symbol, Arrays.stream(values())
                                                                          .map(u -> u.symbol)
                                                                          .collect(Collectors.joining(", "))));

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

@@ -1379,16 +1379,7 @@ public static void setPermissionsUpdateInterval(int updateInterval)
         if (updateInterval == -1)
             conf.permissions_update_interval = null;
         else
-        {
-            try
-            {
-                conf.permissions_update_interval = SmallestDurationMilliseconds.inMilliseconds(updateInterval);
-            }
-            catch (ConfigurationException e)
-            {
-                throw new IllegalArgumentException("permissions_update_interval should be >= -1");
-            }
-        }
+            conf.permissions_update_interval = SmallestDurationMilliseconds.inMilliseconds(updateInterval);
     }
 
     public static int getPermissionsCacheMaxEntries()
@@ -1443,16 +1434,7 @@ public static void setRolesUpdateInterval(int interval)
         if (interval == -1)
             conf.roles_update_interval = null;
         else
-        {
-            try
-            {
-                conf.roles_update_interval = SmallestDurationMilliseconds.inMilliseconds(interval);
-            }
-            catch(ConfigurationException e)
-            {
-                throw new IllegalArgumentException("roles_update_interval should be >= -1");
-            }
-        }
+            conf.roles_update_interval = SmallestDurationMilliseconds.inMilliseconds(interval);
     }
 
     public static int getRolesCacheMaxEntries()
@@ -1487,16 +1469,7 @@ public static void setCredentialsUpdateInterval(int updateInterval)
         if (updateInterval == -1)
             conf.credentials_update_interval = null;
         else
-        {
-            try
-            {
-                conf.credentials_update_interval = SmallestDurationMilliseconds.inMilliseconds(updateInterval);
-            }
-            catch (ConfigurationException e)
-            {
-                throw new IllegalArgumentException("credentials_update_interval should be >= -1.");
-            }
-        }
+            conf.credentials_update_interval = SmallestDurationMilliseconds.inMilliseconds(updateInterval);
     }
 
     public static int getCredentialsCacheMaxEntries()
@@ -2669,14 +2642,7 @@ public static void setNativeTransportMaxRequestDataInFlightPerIpInBytes(long max
         if (maxRequestDataInFlightInBytes == -1)
             maxRequestDataInFlightInBytes = Runtime.getRuntime().maxMemory() / 40;
 
-        try
-        {
-            conf.native_transport_max_request_data_in_flight_per_ip = DataStorageSpec.inBytes(maxRequestDataInFlightInBytes);
-        }
-        catch (ConfigurationException e)
-        {
-            throw new IllegalArgumentException("native_transport_max_request_data_in_flight_per_ip can be only -1 which gets default value or >= 0");
-        }
+        conf.native_transport_max_request_data_in_flight_per_ip = DataStorageSpec.inBytes(maxRequestDataInFlightInBytes);
 
     }
 
@@ -2690,14 +2656,7 @@ public static void setNativeTransportConcurrentRequestDataInFlightInBytes(long m
         if (maxRequestDataInFlightInBytes == -1)
             maxRequestDataInFlightInBytes = Runtime.getRuntime().maxMemory() / 10;
 
-        try
-        {
-            conf.native_transport_max_request_data_in_flight = DataStorageSpec.inBytes(maxRequestDataInFlightInBytes);
-        }
-        catch (ConfigurationException e)
-        {
-            throw new IllegalArgumentException("native_transport_max_request_data_in_flight can be only -1 which gets default value or >= 0");
-        }
+        conf.native_transport_max_request_data_in_flight = DataStorageSpec.inBytes(maxRequestDataInFlightInBytes);
     }
 
     public static int getNativeTransportMaxRequestsPerSecond()
@@ -4097,16 +4056,16 @@ public static void setRowIndexReadSizeFailThreshold(@Nullable DataStorageSpec va
 
     public static int getDefaultKeyspaceRF() { return conf.default_keyspace_rf; }
 
-    public static void setDefaultKeyspaceRF(int value) throws ConfigurationException
+    public static void setDefaultKeyspaceRF(int value) throws IllegalArgumentException
     {
         if (value < 1)
         {
-            throw new ConfigurationException("default_keyspace_rf cannot be less than 1");
+            throw new IllegalArgumentException("default_keyspace_rf cannot be less than 1");
         }
 
         if (value < guardrails.getMinimumReplicationFactorFailThreshold())
         {
-            throw new ConfigurationException(String.format("default_keyspace_rf to be set (%d) cannot be less than minimum_replication_factor_fail_threshold (%d)", value, guardrails.getMinimumReplicationFactorFailThreshold()));
+            throw new IllegalArgumentException(String.format("default_keyspace_rf to be set (%d) cannot be less than minimum_replication_factor_fail_threshold (%d)", value, guardrails.getMinimumReplicationFactorFailThreshold()));
         }
 
         conf.default_keyspace_rf = value;

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

@@ -28,8 +28,6 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.primitives.Ints;
 
-import org.apache.cassandra.exceptions.ConfigurationException;
-
 import static java.util.concurrent.TimeUnit.DAYS;
 import static java.util.concurrent.TimeUnit.HOURS;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
@@ -73,15 +71,15 @@ public DurationSpec(String value)
         }
         else
         {
-            throw new ConfigurationException("Invalid duration: " + value + " Accepted units: d, h, m, s, ms, us, µs," +
+            throw new IllegalArgumentException("Invalid duration: " + value + " Accepted units: d, h, m, s, ms, us, µs," +
                                              " ns where case matters and " + "only non-negative values");
         }
     }
 
     DurationSpec(long quantity, TimeUnit unit)
     {
         if (quantity < 0)
-            throw new ConfigurationException("Invalid duration " + quantity + ": value must be positive");
+            throw new IllegalArgumentException("Invalid duration " + quantity + ": value must be positive");
 
         this.quantity = quantity;
         this.unit = unit;
@@ -95,7 +93,7 @@ public DurationSpec(String value)
     public DurationSpec(String value, TimeUnit minUnit)
     {
         if (!MAP_UNITS_PER_MIN_UNIT.containsKey(minUnit))
-            throw new ConfigurationException("Invalid smallest unit set for " + value);
+            throw new IllegalArgumentException("Invalid smallest unit set for " + value);
 
         Matcher matcher = TIME_UNITS_PATTERN.matcher(value);
 
@@ -105,11 +103,11 @@ public DurationSpec(String value, TimeUnit minUnit)
             unit = fromSymbol(matcher.group(2));
 
             if (!MAP_UNITS_PER_MIN_UNIT.get(minUnit).contains(unit))
-                throw new ConfigurationException("Invalid duration: " + value + " Accepted units:" + MAP_UNITS_PER_MIN_UNIT.get(minUnit));
+                throw new IllegalArgumentException("Invalid duration: " + value + " Accepted units:" + MAP_UNITS_PER_MIN_UNIT.get(minUnit));
         }
         else
         {
-            throw new ConfigurationException("Invalid duration: " + value + " Accepted units:" + MAP_UNITS_PER_MIN_UNIT.get(minUnit) +
+            throw new IllegalArgumentException("Invalid duration: " + value + " Accepted units:" + MAP_UNITS_PER_MIN_UNIT.get(minUnit) +
                                              " where case matters and only non-negative values.");
         }
     }
@@ -229,7 +227,7 @@ static TimeUnit fromSymbol(String symbol)
             case "µs": return TimeUnit.MICROSECONDS;
             case "ns": return TimeUnit.NANOSECONDS;
         }
-        throw new ConfigurationException(String.format("Unsupported time unit: %s. Supported units are: %s",
+        throw new IllegalArgumentException(String.format("Unsupported time unit: %s. Supported units are: %s",
                                                        symbol, Arrays.stream(TimeUnit.values())
                                                                      .map(DurationSpec::getSymbol)
                                                                      .collect(Collectors.joining(", "))));

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

@@ -21,8 +21,6 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
-import java.nio.file.Files;
-import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
@@ -31,8 +29,6 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.Lists;
@@ -116,7 +112,6 @@ public Config loadConfig() throws ConfigurationException
         if (storageConfigURL == null)
             storageConfigURL = getStorageConfigURL();
 
-        validateConfigFile();
         return loadConfig(storageConfigURL);
     }
 
@@ -210,46 +205,6 @@ private static void verifyReplacements(Map<Class<?>, Map<String, Replacement>> r
 
     }
 
-    private static String readStorageConfig(URL url)
-    {
-        String content;
-
-        try
-        {
-            content = new String (Files.readAllBytes(Paths.get(String.valueOf(url).substring(5))));
-        }
-        catch (IOException e)
-        {
-            throw new ConfigurationException("Invalid yaml: " + url, e);
-        }
-
-        return content;
-    }
-
-    private static void validateConfigFile()
-    {
-        String content = YamlConfigurationLoader.readStorageConfig(storageConfigURL);
-
-        if (isBlank("commitlog_sync_period", content))
-            throw new IllegalArgumentException("You should provide a value for commitlog_sync_period or comment it in " +
-                                               "order to get a default one");
-
-        if (isBlank("commitlog_sync_group_window", content))
-            throw new IllegalArgumentException("You should provide a value for commitlog_sync_group_window or comment it in " +
-                                               "order to get a default one");
-    }
-
-    /**
-     * This method helps to preserve the behavior of parameters which were originally of primitive type and
-     * without default value in Config.java (CASSANDRA-15234)
-     */
-    private static boolean isBlank(String property, String content)
-    {
-        Pattern p = Pattern.compile(String.format("%s%s *: *$", '^', property), Pattern.MULTILINE);
-        Matcher m = p.matcher(content);
-        return m.find();
-    }
-
     @VisibleForTesting
     public static <T> T fromMap(Map<String,Object> map, Class<T> klass)
     {

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

@@ -19,7 +19,6 @@
 
 import org.junit.Test;
 
-import org.apache.cassandra.exceptions.ConfigurationException;
 import org.quicktheories.core.Gen;
 import org.quicktheories.generators.SourceDSL;
 
@@ -72,18 +71,18 @@ public void testFromSymbol()
         assertEquals(DataRateSpec.DataRateUnit.fromSymbol("KiB/s"), DataRateSpec.DataRateUnit.KIBIBYTES_PER_SECOND);
         assertEquals(DataRateSpec.DataRateUnit.fromSymbol("MiB/s"), DataRateSpec.DataRateUnit.MEBIBYTES_PER_SECOND);
         assertThatThrownBy(() -> DataRateSpec.DataRateUnit.fromSymbol("n"))
-        .isInstanceOf(ConfigurationException.class)
+        .isInstanceOf(IllegalArgumentException.class)
         .hasMessageContaining("Unsupported data rate unit: n");
     }
 
     @Test
     public void testInvalidInputs()
     {
-        assertThatThrownBy(() -> new DataRateSpec("10")).isInstanceOf(ConfigurationException.class)
+        assertThatThrownBy(() -> new DataRateSpec("10")).isInstanceOf(IllegalArgumentException.class)
                                                         .hasMessageContaining("Invalid bit rate: 10");
-        assertThatThrownBy(() -> new DataRateSpec("-10b/s")).isInstanceOf(ConfigurationException.class)
+        assertThatThrownBy(() -> new DataRateSpec("-10b/s")).isInstanceOf(IllegalArgumentException.class)
                                                             .hasMessageContaining("Invalid bit rate: -10b/s");
-        assertThatThrownBy(() -> new DataRateSpec("10xb/s")).isInstanceOf(ConfigurationException.class)
+        assertThatThrownBy(() -> new DataRateSpec("10xb/s")).isInstanceOf(IllegalArgumentException.class)
                                                             .hasMessageContaining("Invalid bit rate: 10xb/s");
         assertThatThrownBy(() -> new DataRateSpec("9223372036854775809B/s")
                                  .toBytesPerSecond()).isInstanceOf(NumberFormatException.class)