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

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

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


##########
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:
   batch_size_fail_threshold doesn't need the validation and I assumed this part of the patch will be removed.
   Anyway, I am not against adding a nice utility method and agree with @adelapena's review comments.
   
   > One last thought about this is that perhaps we should move this validation into DataStorageSpec.IntKibibytesBound, maybe with a validation method or with a static constructor performing the validation. But we don't have to do that during this ticket. 
   
   Didn't we intentionally leave it out of those classes and opted out of custom static methods and constructors as it is a corner case for only a few parameters?  I thought we didn't want to encourage too much customizations in those classes to ensure we keep them clean, only in case we consider this will be a very common case.



-- 
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