[GitHub] [cassandra] adelapena commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold

[GitBox <gi...@apache.org>]

adelapena commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r902597039


##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
 
     public static void setBatchSizeWarnThresholdInKiB(int threshold)
     {
-        DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
-        checkValidForByteConversion(storage, "batch_size_warn_threshold");
-        conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+        conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
     }
 
     public static void setBatchSizeFailThresholdInKiB(int threshold)
     {
-        conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+        conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+    }
+
+    private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){

Review Comment:
   Nit: the final brace should be on a new line, K&R style:
   ```suggestion
       private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName)
       {
   ```
   Also, the `byteValue` argument is meant to contain a value in kibibytes, so we should probably name it `kibibyteValue`. Or maybe just `kibibytes`, as it's named in the`DataStorageSpec.IntKibibytesBound` constructor that we use immediately below:
   ```suggestion
       private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int kibibytes, String configName)
       {
   ```
   I would also rename the `configName` argument to `propertyName`, I think:
   ```suggestion
       private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int kibibytes, String propertyName)
       {
   ```
   I wonder if we should put this new method and the related `checkValidForByteConversion` closer to each other in the file, wdyt?
   
   One last thought about this is that perhaps we should move this validation into `DataStorageSpec.IntKibibytesBound`, maybe with a validation method or with static constructor performing the validation. But we don't have to do that during this ticket.



##########
test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java:
##########
@@ -326,7 +326,24 @@ public void testExceptionsForInvalidConfigValues() {
             fail("Should have received a ConfigurationException batch_size_warn_threshold = 2GiB");
         }
         catch (ConfigurationException ignored) { }
-        Assert.assertEquals(4096, DatabaseDescriptor.getColumnIndexSize());

Review Comment:
   Good catch!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org