reflected review

Author: smiklosovic

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

@@ -1221,11 +1221,21 @@ else if (conf.max_value_size.toMebibytes() >= 2048)
         if (conf.audit_logging_options != null)
             setAuditLoggingOptions(conf.audit_logging_options);
 
-        // just run through the validation by setting current values back to their setters, so we are sure that their values are valid
-        DatabaseDescriptor.setGCLogThreshold((int) DatabaseDescriptor.getGCLogThreshold());
-        DatabaseDescriptor.setGCWarnThreshold((int) DatabaseDescriptor.getGCWarnThreshold());
-        DatabaseDescriptor.setGCConcurrentPhaseLogThreshold(DatabaseDescriptor.getGCConcurrentPhaseLogThreshold());
-        DatabaseDescriptor.setGCConcurrentPhaseWarnThreshold(DatabaseDescriptor.getGCConcurrentPhaseWarnThreshold());
+        try
+        {
+            // Run through the validation by setting current values back to their setters, so we are sure that their values are valid.
+            // We are catching IllegalArgumentException and translating it to ConfigurationException to comply with
+            // rest of the logic in this method. These setters are also called in GCInspectorMXBean were IllegalArgumentException
+            // is thrown when arguments are invalid instead ConfigurationException, on purpose.
+            DatabaseDescriptor.setGCLogThreshold((int) DatabaseDescriptor.getGCLogThreshold());
+            DatabaseDescriptor.setGCWarnThreshold((int) DatabaseDescriptor.getGCWarnThreshold());
+            DatabaseDescriptor.setGCConcurrentPhaseLogThreshold(DatabaseDescriptor.getGCConcurrentPhaseLogThreshold());
+            DatabaseDescriptor.setGCConcurrentPhaseWarnThreshold(DatabaseDescriptor.getGCConcurrentPhaseWarnThreshold());
+        }
+        catch (IllegalArgumentException ex)
+        {
+            throw new ConfigurationException(ex.getMessage());
+        }
     }
 
     @VisibleForTesting
@@ -4728,12 +4738,12 @@ public static long getGCLogThreshold()
     public static void setGCLogThreshold(int threshold)
     {
         if (threshold <= 0)
-            throw new ConfigurationException("Threshold value for gc_log_threshold must be greater than 0");
+            throw new IllegalArgumentException("Threshold value for gc_log_threshold must be greater than 0");
 
         long gcWarnThresholdInMs = getGCWarnThreshold();
         if (gcWarnThresholdInMs != 0 && threshold > gcWarnThresholdInMs)
-            throw new ConfigurationException("Threshold value for gc_log_threshold (" + threshold + ") must be less than gc_warn_threshold which is currently "
-                                             + gcWarnThresholdInMs);
+            throw new IllegalArgumentException("Threshold value for gc_log_threshold (" + threshold + ") must be less than gc_warn_threshold which is currently "
+                                               + gcWarnThresholdInMs);
 
         conf.gc_log_threshold = new DurationSpec.IntMillisecondsBound(threshold);
     }
@@ -4746,12 +4756,12 @@ public static long getGCWarnThreshold()
     public static void setGCWarnThreshold(int threshold)
     {
         if (threshold < 0)
-            throw new ConfigurationException("Threshold value for gc_warn_threshold must be greater than or equal to 0");
+            throw new IllegalArgumentException("Threshold value for gc_warn_threshold must be greater than or equal to 0");
 
         long gcLogThresholdInMs = getGCLogThreshold();
         if (threshold != 0 && threshold <= gcLogThresholdInMs)
-            throw new ConfigurationException("Threshold value for gc_warn_threshold (" + threshold + ") must be greater than gc_log_threshold which is currently "
-                                             + gcLogThresholdInMs);
+            throw new IllegalArgumentException("Threshold value for gc_warn_threshold (" + threshold + ") must be greater than gc_log_threshold which is currently "
+                                               + gcLogThresholdInMs);
 
         conf.gc_warn_threshold = new DurationSpec.IntMillisecondsBound(threshold);
     }
@@ -4764,12 +4774,12 @@ public static int getGCConcurrentPhaseLogThreshold()
     public static void setGCConcurrentPhaseLogThreshold(int threshold)
     {
         if (threshold <= 0)
-            throw new ConfigurationException("Threshold must be greater than 0");
+            throw new IllegalArgumentException("Threshold must be greater than 0");
 
         long gcConcurrentPhaseWarnThresholdInMs = getGCConcurrentPhaseWarnThreshold();
         if (gcConcurrentPhaseWarnThresholdInMs != 0 && threshold > gcConcurrentPhaseWarnThresholdInMs)
-            throw new ConfigurationException("Threshold value for gc_concurrent_phase_log_threshold (" + threshold + ") must be less than gc_concurrent_phase_warn_threshold which is currently "
-                                             + gcConcurrentPhaseWarnThresholdInMs);
+            throw new IllegalArgumentException("Threshold value for gc_concurrent_phase_log_threshold (" + threshold + ") must be less than gc_concurrent_phase_warn_threshold which is currently "
+                                               + gcConcurrentPhaseWarnThresholdInMs);
 
         conf.gc_concurrent_phase_log_threshold = new DurationSpec.IntMillisecondsBound(threshold);
     }
@@ -4782,12 +4792,12 @@ public static int getGCConcurrentPhaseWarnThreshold()
     public static void setGCConcurrentPhaseWarnThreshold(int threshold)
     {
         if (threshold < 0)
-            throw new ConfigurationException("Threshold value for gc_concurrent_phase_warn_threshold must be greater than or equal to 0");
+            throw new IllegalArgumentException("Threshold value for gc_concurrent_phase_warn_threshold must be greater than or equal to 0");
 
         long gcConcurrentPhaseLogThresholdInMs = getGCConcurrentPhaseLogThreshold();
         if (threshold != 0 && threshold <= gcConcurrentPhaseLogThresholdInMs)
-            throw new ConfigurationException("Threshold value for gc_concurrent_phase_warn_threshold (" + threshold + ") must be greater than gc_concurrent_phase_log_threshold which is currently "
-                                             + gcConcurrentPhaseLogThresholdInMs);
+            throw new IllegalArgumentException("Threshold value for gc_concurrent_phase_warn_threshold (" + threshold + ") must be greater than gc_concurrent_phase_log_threshold which is currently "
+                                               + gcConcurrentPhaseLogThresholdInMs);
 
         conf.gc_concurrent_phase_warn_threshold = new DurationSpec.IntMillisecondsBound(threshold);
     }

src/java/org/apache/cassandra/service/GCInspector.java

@@ -296,6 +296,7 @@ else if (duration > getGcConcurrentPhaseLogThresholdInMs())
                     logger.info(sb.toString());
                 else if (logger.isTraceEnabled())
                     logger.trace(sb.toString());
+
                 if (duration > this.getConcurrentStatusThresholdInMs())
                     StatusLogger.log();
             }
@@ -307,6 +308,7 @@ else if (duration > getGcLogThresholdInMs())
                     logger.info(sb.toString());
                 else if (logger.isTraceEnabled())
                     logger.trace(sb.toString());
+
                 if (duration > this.getStatusThresholdInMs())
                     StatusLogger.log();
             }
@@ -409,14 +411,7 @@ private static long getFieldValue(Field field, boolean isAtomicLong)
 
     public void setGcWarnThresholdInMs(long threshold)
     {
-        try
-        {
-            DatabaseDescriptor.setGCWarnThreshold((int)threshold);
-        }
-        catch (Throwable t)
-        {
-            throw new IllegalArgumentException(t.getMessage());
-        }
+        DatabaseDescriptor.setGCWarnThreshold((int)threshold);
     }
 
     public long getGcWarnThresholdInMs()
@@ -426,14 +421,7 @@ public long getGcWarnThresholdInMs()
 
     public void setGcLogThresholdInMs(long threshold)
     {
-        try
-        {
-            DatabaseDescriptor.setGCLogThreshold((int) threshold);
-        }
-        catch (Throwable t)
-        {
-            throw new IllegalArgumentException(t.getMessage());
-        }
+        DatabaseDescriptor.setGCLogThreshold((int) threshold);
     }
 
     public int getGcConcurrentPhaseWarnThresholdInMs()
@@ -443,14 +431,7 @@ public int getGcConcurrentPhaseWarnThresholdInMs()
 
     public void setGcConcurrentPhaseWarnThresholdInMs(int threshold)
     {
-        try
-        {
-            DatabaseDescriptor.setGCConcurrentPhaseWarnThreshold(threshold);
-        }
-        catch (Throwable t)
-        {
-            throw new IllegalArgumentException(t.getMessage());
-        }
+        DatabaseDescriptor.setGCConcurrentPhaseWarnThreshold(threshold);
     }
 
     public int getGcConcurrentPhaseLogThresholdInMs()
@@ -460,14 +441,7 @@ public int getGcConcurrentPhaseLogThresholdInMs()
 
     public void setGcConcurrentPhaseLogThresholdInMs(int threshold)
     {
-        try
-        {
-            DatabaseDescriptor.setGCConcurrentPhaseLogThreshold(threshold);
-        }
-        catch (Throwable t)
-        {
-            throw new IllegalArgumentException(t.getMessage());
-        }
+        DatabaseDescriptor.setGCConcurrentPhaseLogThreshold(threshold);
     }
 
     public long getGcLogThresholdInMs()