Claudenw commented on code in PR #4087:
URL: https://github.com/apache/cassandra/pull/4087#discussion_r2040633507
##########
src/java/org/apache/cassandra/db/compaction/unified/Controller.java:
##########
@@ -519,6 +519,11 @@ public static Map<String, String>
validateOptions(Map<String, String> options) t
try
{
targetSSTableSize = FBUtilities.parseHumanReadableBytes(s);
+ 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));
+ }
Review Comment:
There are several problems with the code.
1. 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.
2. If the double value is less than MIN_TARGET_SSTABLE_SIZE you can throw an
exception.
3. 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);`)
##########
test/unit/org/apache/cassandra/db/compaction/unified/ControllerTest.java:
##########
@@ -121,6 +121,18 @@ public void testValidateOptionsIntegers()
testValidateOptions(true);
}
+ @Test
+ public void testValidateOptionsInvalidTargetSSTableSize()
+ {
+ try {
+ Map<String, String> options = new HashMap<>();
+ options.putIfAbsent(Controller.TARGET_SSTABLE_SIZE_OPTION,
"12E899");
+ Controller.validateOptions(options);
+ fail("Passing invalid number should have caused exception");
+ } catch(ConfigurationException e) {
+ }
+ }
+
Review Comment:
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.
##########
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",
Review Comment:
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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
