[GitHub] [cassandra] adelapena commented on a diff in pull request #1758: 17737 4.1 sstable preemptive open interval in mb

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

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


##########
.build/build-rat.xml:
##########
@@ -58,6 +58,8 @@
                  <exclude NAME="**/doc/antora.yml"/>
                  <exclude name="**/test/conf/cassandra.yaml"/>
                  <exclude name="**/test/conf/cassandra-old.yaml"/>
+                 <exclude name="**/cassandra-converters-special-cases-old-names.yaml"/>

Review Comment:
   I think this should be `<exclude name="**/test/conf/cassandra-converters-special-cases-old-names.yaml"/>`



##########
test/conf/cassandra-converters-special-cases-old-names.yaml:
##########
@@ -0,0 +1,3 @@
+sstable_preemptive_open_interval_in_mb: -1

Review Comment:
   Maybe we could add a brief comment at the beginning of this file indicating what it's used for.



##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -423,7 +425,8 @@ public MemtableOptions()
     @Replaces(oldName = "trickle_fsync_interval_in_kb", converter = Converters.KIBIBYTES_DATASTORAGE, deprecated = true)
     public DataStorageSpec.IntKibibytesBound trickle_fsync_interval = new DataStorageSpec.IntKibibytesBound("10240KiB");
 
-    @Replaces(oldName = "sstable_preemptive_open_interval_in_mb", converter = Converters.MEBIBYTES_DATA_STORAGE_INT, deprecated = true)
+    @Nullable
+    @Replaces(oldName = "sstable_preemptive_open_interval_in_mb", converter = Converters.NEGATIVE_DATA_STORAGE_INT, deprecated = true)

Review Comment:
   Should't this keep including `MEBIBYTES` in the name, as in `NEGATIVE_MEBIBYTES_DATA_STORAGE_INT` or `MEBIBYTES_CUSTOM_DATA_STORAGE_INT`?



##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -3074,12 +3074,17 @@ public static void setMigrateKeycacheOnCompaction(boolean migrateCacheEntry)
 
     public static int getSSTablePreemptiveOpenIntervalInMiB()
     {
+        if (conf.sstable_preemptive_open_interval == null)

Review Comment:
   I'd add some JavaDoc to the getter mentioning that it might return negative.



##########
test/conf/cassandra-converters-special-cases.yaml:
##########
@@ -0,0 +1,3 @@
+sstable_preemptive_open_interval:

Review Comment:
   Maybe we could add a brief comment at the beginning of this file indicating what it's used for.



##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -3074,12 +3074,17 @@ public static void setMigrateKeycacheOnCompaction(boolean migrateCacheEntry)
 
     public static int getSSTablePreemptiveOpenIntervalInMiB()
     {
+        if (conf.sstable_preemptive_open_interval == null)
+            return -1;
         return conf.sstable_preemptive_open_interval.toMebibytes();
     }
 
     public static void setSSTablePreemptiveOpenIntervalInMiB(int mib)
     {
-        conf.sstable_preemptive_open_interval = new DataStorageSpec.IntMebibytesBound(mib);
+        if (mib < 0)

Review Comment:
   I'd add some JavaDoc to this setter mentioning that it accepts negative values.



##########
NEWS.txt:
##########
@@ -165,6 +165,9 @@ New features
 
 Upgrading
 ---------
+    - `sstable_preemptive_open_interval_in_mb` being negatve for disabled is equivalent to `sstable_preemptive_open_interval`

Review Comment:
   ```suggestion
       - `sstable_preemptive_open_interval_in_mb` being negative for disabled is equivalent to `sstable_preemptive_open_interval`
   ```



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