(CASSANDRA-20398) targetSSTableSize Long.MAX_VALUE/Long.MIN_VALUE check #4087

pranavshenoy · 2025-04-12T05:51:05Z

patch by Pranav Shenoy; reviewed by Claude Warren for CASSANDRA-20398

Claudenw · 2025-04-12T11:02:50Z

src/java/org/apache/cassandra/db/compaction/unified/Controller.java

+                if (targetSSTableSize == Long.MAX_VALUE || targetSSTableSize == Long.MIN_VALUE) {
+                    throw new ConfigurationException(String.format("%s %s is out of range of Long.",
+                                                                    TARGET_SSTABLE_SIZE_OPTION,
+                                                                    s));
+                }


There are several problems with the code.

FBUtils.parseHumanReadalbeBytes(s) converts s to a double and then casts it as a long. So the upper limit is Long.MAX_VALUE. However sufficiently large values of s will cause a truncation issue. If you want to report that the value is too large you can use FBUtilities.parseHumanReadable() (see FBUtilities.parseHumanReadableBytes() for an example call) this will return a double which you can then check to see if it is larger than MAX_LONG. Your current test will only fail if s is exactly Long.MAX_VALUE or Long.MIN_VALUE. I am not certain you need to check for the min value as any value less than 0 should be an error.

If the double value is less than MIN_TARGET_SSTABLE_SIZE you can throw an exception.

Once you have validate that the double value is within the acceptable range for the long you can do the case (as seen in FBUtilities.parseHumanReadableBytes(s);)

@Claudenw Thanks for the reviewing my PR.

I have made the changes to the current PR.

Claudenw · 2025-04-12T11:06:54Z

src/java/org/apache/cassandra/db/compaction/unified/Controller.java

@@ -624,7 +629,7 @@ public static Map<String, String> validateOptions(Map<String, String> options) t
                                                                   MIN_SSTABLE_SIZE_OPTION));
                int limit = (int) Math.ceil(targetSSTableSize * INVERSE_SQRT_2);
                if (sizeInBytes >= limit)
-                    throw new ConfigurationException(String.format("Invalid configuration, %s (%s) should be less than the target size minimum: %s",
+                    throw new ConfigurationException(String.format("Invalid configuration, %s (%s) should be less than 70% of the target size minimum: %s",


The issue here is much the same as the double to long conversion issue. The problem again is the casting causing an invalid value. at line 630, rather than converting directly to int do the calculation using a long. Then verify that the values are in range.

Changed the type to long and have added the test case for this. Thanks

Let me know your thoughts.

Claudenw · 2025-04-12T11:51:18Z

test/unit/org/apache/cassandra/db/compaction/unified/ControllerTest.java

+        } catch(ConfigurationException e) {
+        }
+    }
+


What I want to see in the test cases to begin with are the values from the issue. Show that they fail the test first so that you know you have identified the issue.

Write multiple tests. You want to include values of s that are greater than Long.MAX_VALUE for both of the cases you are trying to capture. Also, for the code before your changes a TARGET_SSTABLE_SIZE_OPTION of 3650722199 and a MIN_SS_TABLE_SIZE option of "2581450423" should produce an error.

0.7071067811865475 = 1/Sqrt(2) 2581450424 = (long) Math.ceil(3650722199 * 0.7071067811865475) 2147483647 = (int) Math.ceil(3650722199 * 0.7071067811865475)

so any value for MIN_SS_TABLE_SIZE in the reange [21474836487, 2581450423] should fail before the change and pass after.

added these test cases. Thanks

Please rename the tests to indicate what they are testing. For example
testValidateOptionsTargetSSTableSize1() could be renamed testCassandra20398Values()
and
testValidateOptionsTargetSSTableSize2() could be renamed testTargetSstableSizeOptionGTLongMax()

In fact you could combine 1 and 2 into a single parameter driven method that uses "12E899 B" and "9223372036854775907 B" as values and then in comments explain that 12E899B comes from CASSANDRA-20398 and 9223372036854775907 is Long.MAX_VALUE + 100

Question: Why does Long.MAX_VALUE + 100 fail?

A little experimentation shows that
9223372036854776000 is what Long.MAX_VALUE converts to as a double.

all values from [Long.MAX_VALUE, Long.MAX_VALUE + 1024] convert to that value so all of them should be considered valid.

There needs to be a test for exactly Long.MAX_VALUE to show that it passes.

You could write a single paramaterized test that takes the configuration file formatted value (e.g. "9223372036854775907 B") and a boolean flag to indicate that it should or should not pass the test. Then we can add other values as it becomes clear that they need to be tested.

@Claudenw have changed the name of the testcase and created a common function for both test cases.

Thanks for sharing this interesting insight on converting Long.MAX_VALUE to double.

In order to understand more, I ran this code:
double d = Long.MAX_VALUE; if(d > Long.MAX_VALUE) { System.out.println("d>Long.MAX_VALUE"); } else if(d == Long.MAX_VALUE) { System.out.println("d == Long.MAX_VALUE"); } else { System.out.println("d < Long.MAX_VALUE"); } System.out.println(d-Long.MAX_VALUE);

When I print d, I get 9223372036854776000. However, the output of the code was
d == Long.MAX_VALUE and d-Long.MAX_VALUE gives 0 too.

From this, I feel like, during any condition checks/operations, d will have Long.MAX_VALUE. Correct me if I am wrong.

src/java/org/apache/cassandra/db/compaction/unified/Controller.java

test/unit/org/apache/cassandra/db/compaction/unified/ControllerTest.java

dcapwell · 2025-04-16T22:06:00Z

src/java/org/apache/cassandra/db/compaction/unified/Controller.java

                if (sizeInBytes >= limit)
-                    throw new ConfigurationException(String.format("Invalid configuration, %s (%s) should be less than the target size minimum: %s",


We report 2 variables to the user

limit - this is computed from targetSSTableSize; targetSSTableSize is what the user gave

*sizeInBytes - this is what the user gave

Is there a reason for not showing the user targetSSTableSize? I am told that my configs don't match, but I don't know what 2 configs; i only know one of them

I agree with dcapwell here as well. The exception string might say something like "Invalid configuration, MIN_SSTABLE_SIZE_OPTION (%s) should be less than sizeInBytes, which is calculated as 70% of the targetSSTableSize (%s)"

have made the changes. I modified the log and removed sizeInBytes as it is computed internally. Now the log looks like this:
Invalid configuration, MIN_SSTABLE_SIZE_OPTION (%s) should be less than 70%% of the targetSSTableSize (targetSSTableSize)

let me know if this is fine.

src/java/org/apache/cassandra/db/compaction/unified/Controller.java

Claudenw · 2025-04-18T06:43:01Z

src/java/org/apache/cassandra/db/compaction/unified/Controller.java

                if (sizeInBytes >= limit)
-                    throw new ConfigurationException(String.format("Invalid configuration, %s (%s) should be less than the target size minimum: %s",


I agree with dcapwell here as well. The exception string might say something like "Invalid configuration, MIN_SSTABLE_SIZE_OPTION (%s) should be less than sizeInBytes, which is calculated as 70% of the targetSSTableSize (%s)"

Claudenw · 2025-04-18T07:16:36Z

test/unit/org/apache/cassandra/db/compaction/unified/ControllerTest.java

+        } catch(ConfigurationException e) {
+        }
+    }
+


Please rename the tests to indicate what they are testing. For example
testValidateOptionsTargetSSTableSize1() could be renamed testCassandra20398Values()
and
testValidateOptionsTargetSSTableSize2() could be renamed testTargetSstableSizeOptionGTLongMax()

In fact you could combine 1 and 2 into a single parameter driven method that uses "12E899 B" and "9223372036854775907 B" as values and then in comments explain that 12E899B comes from CASSANDRA-20398 and 9223372036854775907 is Long.MAX_VALUE + 100

Question: Why does Long.MAX_VALUE + 100 fail?

A little experimentation shows that
9223372036854776000 is what Long.MAX_VALUE converts to as a double.

all values from [Long.MAX_VALUE, Long.MAX_VALUE + 1024] convert to that value so all of them should be considered valid.

There needs to be a test for exactly Long.MAX_VALUE to show that it passes.

You could write a single paramaterized test that takes the configuration file formatted value (e.g. "9223372036854775907 B") and a boolean flag to indicate that it should or should not pass the test. Then we can add other values as it becomes clear that they need to be tested.

…LE_SIZE_OPTION The values were getting truncated resulting in unexpected output. patch by Pranav Shenoy; reviewed by Claude Warren for CASSANDRA-20398

Claudenw suggested changes Apr 12, 2025

View reviewed changes

pranavshenoy force-pushed the CASSANDRA-20398 branch from 504be8c to 72a742c Compare April 13, 2025 02:14

pranavshenoy requested a review from Claudenw April 13, 2025 02:18