Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/java/org/apache/cassandra/db/compaction/unified/Controller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,14 +518,20 @@ public static Map<String, String> validateOptions(Map<String, String> options) t
{
try
{
targetSSTableSize = FBUtilities.parseHumanReadableBytes(s);
if (targetSSTableSize < MIN_TARGET_SSTABLE_SIZE)
double targetSize = FBUtilities.parseHumanReadable(s, null, "B");
if (targetSize >= Long.MAX_VALUE) {
throw new ConfigurationException(String.format("%s %s is out of range of Long.",
TARGET_SSTABLE_SIZE_OPTION,
s));
}
if (targetSize < MIN_TARGET_SSTABLE_SIZE)
{
throw new ConfigurationException(String.format("%s %s is not acceptable, size must be at least %s",
TARGET_SSTABLE_SIZE_OPTION,
s,
FBUtilities.prettyPrintMemory(MIN_TARGET_SSTABLE_SIZE)));
}
targetSSTableSize = (long) Math.ceil(targetSize);
}
catch (NumberFormatException e)
{
Expand Down Expand Up @@ -622,12 +628,12 @@ public static Map<String, String> validateOptions(Map<String, String> options) t
if (sizeInBytes < 0)
throw new ConfigurationException(String.format("Invalid configuration, %s should be greater than or equal to 0 (zero)",
MIN_SSTABLE_SIZE_OPTION));
int limit = (int) Math.ceil(targetSSTableSize * INVERSE_SQRT_2);
long limit = (long) 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",
Copy link
Contributor

@dcapwell dcapwell Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw new ConfigurationException(String.format("Invalid configuration, %s (%s) should be less than 70%% of the targetSSTableSize (%s)",
MIN_SSTABLE_SIZE_OPTION,
FBUtilities.prettyPrintMemory(sizeInBytes),
FBUtilities.prettyPrintMemory(limit)));
FBUtilities.prettyPrintMemory(targetSSTableSize)));
}
catch (NumberFormatException e)
{
Expand Down
62 changes: 60 additions & 2 deletions test/unit/org/apache/cassandra/db/compaction/unified/ControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,64 @@ public void testValidateOptionsIntegers()
testValidateOptions(true);
}

public void targetSSTableSizeValidator(String inputSize)
{
Map<String, String> options = new HashMap<>();
options.putIfAbsent(Controller.TARGET_SSTABLE_SIZE_OPTION, inputSize);
assertThatExceptionOfType(ConfigurationException.class)
.describedAs("Should have thrown a ConfigurationException when target_sstable_size is greater than Long.MAX_VALUE")
.isThrownBy(() -> Controller.validateOptions(options))
.withMessageContaining(format("target_sstable_size %s is out of range of Long.", inputSize));
}

@Test
public void testCassandra20398Values()
{
//TARGET_SSTABLE_SIZE_OPTION = 12E899, the value reported in CASSANDRA-20398
String inputSize = "12E899 B";
targetSSTableSizeValidator(inputSize);
}

@Test
public void testValidateOptionsTargetSSTableSizeGTLongMax()
{
//TARGET_SSTABLE_SIZE_OPTION > LONG.MAX_VALUE
// the inputSize is Long.MAX_VALUE + 100
String inputSize = "9223372036854775907 B";
targetSSTableSizeValidator(inputSize);
}

@Test
public void testValidateOptionsTargetSSTableSizeLTMinTargetSize()
{
// TARGET_SSTABLE_SIZE_OPTION < Default MIN_TARGET_SSTABLE_SIZE (1048576)
Map<String, String> options = new HashMap<>();
String inputSize = "1048000 B";
options.putIfAbsent(Controller.TARGET_SSTABLE_SIZE_OPTION, inputSize);
assertThatExceptionOfType(ConfigurationException.class)
.describedAs("Should have thrown a ConfigurationException when target_sstable_size is less than default MIN_TARGET_SSTABLE_SIZE")
.isThrownBy(() -> Controller.validateOptions(options))
.withMessageContaining(format("target_sstable_size %s is not acceptable, size must be at least %s", inputSize, FBUtilities.prettyPrintMemory(Controller.MIN_TARGET_SSTABLE_SIZE)));
}

@Test
public void testValidateOptionsTargetSSTableSizeGTIntMax()
{
//TEST 4: Verifying if TARGET_SSTABLE_SIZE_OPTION (3650722199) < MIN_TARGET_SSTABLE_SIZE (2581450423)
// Previously, TARGET_SSTABLE_SIZE_OPTION * 0.7 was stored as Integer which would 3650722199 * 0.7 = 2147483647
// By storing it in a Long, 3650722199 * 0.7 = 2581450424. If TARGET_SSTABLE_SIZE_OPTION * 0.7 is truncated,
//this test case will fail
try
{
Map<String, String> options = new HashMap<>();
options.putIfAbsent(Controller.TARGET_SSTABLE_SIZE_OPTION, "3650722199 B");
options.putIfAbsent(Controller.MIN_SSTABLE_SIZE_OPTION, "2581450423 B");
Controller.validateOptions(options);
} catch(ConfigurationException e) {
fail("3650722199 * 0.7 got truncated. " + e.getMessage());
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added these test cases. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

void testValidateOptions(boolean useIntegers)
{
Map<String, String> options = new HashMap<>();
Expand Down Expand Up @@ -579,7 +637,7 @@ public void testMinSSTableSize()
assertThatExceptionOfType(ConfigurationException.class)
.describedAs("Should have thrown a ConfigurationException when min_sstable_size is greater than target_sstable_size")
.isThrownBy(() -> Controller.validateOptions(options))
.withMessageContaining(format("less than the target size minimum: %s", FBUtilities.prettyPrintMemory(limit)));
.withMessageContaining(format("Invalid configuration, %s (%s) should be less than 70%% of the targetSSTableSize (%s)", Controller.MIN_SSTABLE_SIZE_OPTION, FBUtilities.prettyPrintMemory(limit+1), FBUtilities.prettyPrintMemory(Controller.DEFAULT_TARGET_SSTABLE_SIZE)));

// test min < configured target table size * INV_SQRT_2
limit = (int) Math.ceil(Controller.MIN_TARGET_SSTABLE_SIZE * 2 * Controller.INVERSE_SQRT_2);
Expand All @@ -589,6 +647,6 @@ public void testMinSSTableSize()
assertThatExceptionOfType(ConfigurationException.class)
.describedAs("Should have thrown a ConfigurationException when min_sstable_size is greater than target_sstable_size")
.isThrownBy(() -> Controller.validateOptions(options))
.withMessageContaining(format("less than the target size minimum: %s", FBUtilities.prettyPrintMemory(limit)));
.withMessageContaining(format("Invalid configuration, %s (%s) should be less than 70%% of the targetSSTableSize (%s)", Controller.MIN_SSTABLE_SIZE_OPTION, FBUtilities.prettyPrintMemory(limit + 1), FBUtilities.prettyPrintMemory(Controller.MIN_TARGET_SSTABLE_SIZE * 2)));
}
}