dcapwell commented on code in PR #4228:
URL: https://github.com/apache/cassandra/pull/4228#discussion_r2191180726
##########
test/unit/org/apache/cassandra/cql3/SimpleQueryTest.java:
##########
@@ -561,6 +564,36 @@ public void testSStableTimestampOrdering() throws Throwable
assertRows(execute("SELECT * FROM %s WHERE k1=1"), row(1, 1, 2));
}
+ @Test()
+ public void testInvalidCompactionOptions()
+ {
+ try {
+ createTable("CREATE TABLE %s.test (k int PRIMARY KEY, v int) WITH
compaction = {" +
+ "'class': 'LeveledCompactionStrategy', " +
+ "'fanout_size': '90', " +
+ "'sstable_size_in_mb': '1089'}");
+ }
+ catch (ConfigurationException e) {
+ assertTrue(e.getMessage().contains("your maxSSTableSize must be
absurdly high to compute"));
+ return;
+ }
+ fail("fanout_sizeed and sstable_size_in_mb are invalid, but did not
throw ConfigurationException");
Review Comment:
nit: less code if you move `fail` to the `try` block rather than require the
`return`
##########
test/unit/org/apache/cassandra/cql3/SimpleQueryTest.java:
##########
@@ -561,6 +564,36 @@ public void testSStableTimestampOrdering() throws Throwable
assertRows(execute("SELECT * FROM %s WHERE k1=1"), row(1, 1, 2));
}
+ @Test()
+ public void testInvalidCompactionOptions()
+ {
+ try {
+ createTable("CREATE TABLE %s.test (k int PRIMARY KEY, v int) WITH
compaction = {" +
+ "'class': 'LeveledCompactionStrategy', " +
+ "'fanout_size': '90', " +
+ "'sstable_size_in_mb': '1089'}");
+ }
+ catch (ConfigurationException e) {
+ assertTrue(e.getMessage().contains("your maxSSTableSize must be
absurdly high to compute"));
+ return;
+ }
+ fail("fanout_sizeed and sstable_size_in_mb are invalid, but did not
throw ConfigurationException");
+ }
+
+ @Test()
+ public void testValidCompactionOptions()
+ {
+ try {
+ createTable("CREATE TABLE %s.test (k int PRIMARY KEY, v int) WITH
compaction = {" +
+ "'class': 'LeveledCompactionStrategy', " +
+ "'fanout_size': '10', " +
+ "'sstable_size_in_mb': '160'}");
+ }
+ catch (Exception e) {
+ fail("fanout_sizeed and sstable_size_in_mb are valid, but threw "
+ e.getClass().getName());
Review Comment:
can we remove the try/catch? its cleaner to just let it bubble up than to
recatch and loose context. It also is harder to read as I have to now
understand why we are catching
##########
test/unit/org/apache/cassandra/db/compaction/LeveledCompactionStrategyTest.java:
##########
@@ -969,6 +969,36 @@ public void testReduceScopeL0L1() throws IOException
}
}
+ @Test()
+ public void testInvalidFanoutAndSSTableSize() {
+ try {
+ Map<String, String> options = new HashMap<>();
+ options.put("class", "LeveledCompactionStrategy");
+ options.put("fanout_size", "90");
+ options.put("sstable_size_in_mb", "1089");
+ LeveledCompactionStrategy.validateOptions(options);
+ }
+ catch (ConfigurationException e) {
+ assertTrue(e.getMessage().contains("your maxSSTableSize must be
absurdly high to compute"));
+ return;
+ }
+ Assert.fail("fanout_sizeed and sstable_size_in_mb are invalid, but did
not throw ConfigurationException");
+ }
+
+ @Test
+ public void testValidOptions() {
+ try {
+ Map<String, String> options = new HashMap<>();
+ options.put("class", "LeveledCompactionStrategy");
+ options.put("fanout_size", "10");
+ options.put("sstable_size_in_mb", "160");
+ LeveledCompactionStrategy.validateOptions(options);
+ }
+ catch (Exception e) {
+ Assert.fail("fanout_sizeed and sstable_size_in_mb are valid, but
threw " + e.getClass().getName());
Review Comment:
can we remove the try/catch? its cleaner to just let it bubble up than to
recatch and loose context. It also is harder to read as I have to now
understand why we are catching
##########
src/java/org/apache/cassandra/db/compaction/LeveledCompactionStrategy.java:
##########
@@ -569,10 +570,14 @@ public static Map<String, String>
validateOptions(Map<String, String> options) t
{
Map<String, String> uncheckedOptions =
AbstractCompactionStrategy.validateOptions(options);
+ int ssSize = 1;
+ int fanoutSize = 1;
Review Comment:
can we avoid default values? Makes me double check they are safe, so if we
drop we can let the compiler make sure its safe instead?
##########
test/unit/org/apache/cassandra/cql3/SimpleQueryTest.java:
##########
@@ -561,6 +564,36 @@ public void testSStableTimestampOrdering() throws Throwable
assertRows(execute("SELECT * FROM %s WHERE k1=1"), row(1, 1, 2));
}
+ @Test()
+ public void testInvalidCompactionOptions()
+ {
+ try {
Review Comment:
style: move `{` to the next line
##########
test/unit/org/apache/cassandra/db/compaction/LeveledCompactionStrategyTest.java:
##########
@@ -969,6 +969,36 @@ public void testReduceScopeL0L1() throws IOException
}
}
+ @Test()
+ public void testInvalidFanoutAndSSTableSize() {
Review Comment:
style: move `{` to the next line
##########
test/unit/org/apache/cassandra/db/compaction/LeveledCompactionStrategyTest.java:
##########
@@ -969,6 +969,36 @@ public void testReduceScopeL0L1() throws IOException
}
}
+ @Test()
+ public void testInvalidFanoutAndSSTableSize() {
+ try {
Review Comment:
style: move `{` to the next line
##########
src/java/org/apache/cassandra/db/compaction/LeveledCompactionStrategy.java:
##########
@@ -600,6 +605,19 @@ public static Map<String, String>
validateOptions(Map<String, String> options) t
throw new ConfigurationException(String.format("%s is not a
parsable int (base10) for %s", size, LEVEL_FANOUT_SIZE_OPTION), ex);
}
+ // Validate max Bytes for a level
+ try {
Review Comment:
style: move `{` to the next line
##########
test/unit/org/apache/cassandra/cql3/SimpleQueryTest.java:
##########
@@ -561,6 +564,36 @@ public void testSStableTimestampOrdering() throws Throwable
assertRows(execute("SELECT * FROM %s WHERE k1=1"), row(1, 1, 2));
}
+ @Test()
+ public void testInvalidCompactionOptions()
+ {
+ try {
+ createTable("CREATE TABLE %s.test (k int PRIMARY KEY, v int) WITH
compaction = {" +
+ "'class': 'LeveledCompactionStrategy', " +
+ "'fanout_size': '90', " +
+ "'sstable_size_in_mb': '1089'}");
+ }
+ catch (ConfigurationException e) {
+ assertTrue(e.getMessage().contains("your maxSSTableSize must be
absurdly high to compute"));
+ return;
+ }
+ fail("fanout_sizeed and sstable_size_in_mb are invalid, but did not
throw ConfigurationException");
+ }
+
+ @Test()
+ public void testValidCompactionOptions()
+ {
+ try {
Review Comment:
style: move `{` to the next line
##########
test/unit/org/apache/cassandra/cql3/SimpleQueryTest.java:
##########
@@ -561,6 +564,36 @@ public void testSStableTimestampOrdering() throws Throwable
assertRows(execute("SELECT * FROM %s WHERE k1=1"), row(1, 1, 2));
}
+ @Test()
+ public void testInvalidCompactionOptions()
+ {
+ try {
+ createTable("CREATE TABLE %s.test (k int PRIMARY KEY, v int) WITH
compaction = {" +
+ "'class': 'LeveledCompactionStrategy', " +
+ "'fanout_size': '90', " +
+ "'sstable_size_in_mb': '1089'}");
+ }
+ catch (ConfigurationException e) {
Review Comment:
style: move `{` to the next line
##########
test/unit/org/apache/cassandra/db/compaction/LeveledCompactionStrategyTest.java:
##########
@@ -969,6 +969,36 @@ public void testReduceScopeL0L1() throws IOException
}
}
+ @Test()
+ public void testInvalidFanoutAndSSTableSize() {
+ try {
+ Map<String, String> options = new HashMap<>();
+ options.put("class", "LeveledCompactionStrategy");
+ options.put("fanout_size", "90");
+ options.put("sstable_size_in_mb", "1089");
+ LeveledCompactionStrategy.validateOptions(options);
+ }
+ catch (ConfigurationException e) {
+ assertTrue(e.getMessage().contains("your maxSSTableSize must be
absurdly high to compute"));
+ return;
+ }
+ Assert.fail("fanout_sizeed and sstable_size_in_mb are invalid, but did
not throw ConfigurationException");
+ }
+
+ @Test
+ public void testValidOptions() {
+ try {
Review Comment:
style: move `{` to the next line
##########
test/unit/org/apache/cassandra/db/compaction/LeveledCompactionStrategyTest.java:
##########
@@ -969,6 +969,36 @@ public void testReduceScopeL0L1() throws IOException
}
}
+ @Test()
+ public void testInvalidFanoutAndSSTableSize() {
+ try {
+ Map<String, String> options = new HashMap<>();
+ options.put("class", "LeveledCompactionStrategy");
+ options.put("fanout_size", "90");
+ options.put("sstable_size_in_mb", "1089");
+ LeveledCompactionStrategy.validateOptions(options);
+ }
+ catch (ConfigurationException e) {
+ assertTrue(e.getMessage().contains("your maxSSTableSize must be
absurdly high to compute"));
+ return;
+ }
+ Assert.fail("fanout_sizeed and sstable_size_in_mb are invalid, but did
not throw ConfigurationException");
+ }
+
+ @Test
+ public void testValidOptions() {
Review Comment:
style: move `{` to the next line
##########
test/unit/org/apache/cassandra/db/compaction/LeveledCompactionStrategyTest.java:
##########
@@ -969,6 +969,36 @@ public void testReduceScopeL0L1() throws IOException
}
}
+ @Test()
+ public void testInvalidFanoutAndSSTableSize() {
+ try {
+ Map<String, String> options = new HashMap<>();
+ options.put("class", "LeveledCompactionStrategy");
+ options.put("fanout_size", "90");
+ options.put("sstable_size_in_mb", "1089");
+ LeveledCompactionStrategy.validateOptions(options);
+ }
+ catch (ConfigurationException e) {
Review Comment:
style: move `{` to the next line
##########
test/unit/org/apache/cassandra/db/compaction/LeveledCompactionStrategyTest.java:
##########
@@ -969,6 +969,36 @@ public void testReduceScopeL0L1() throws IOException
}
}
+ @Test()
+ public void testInvalidFanoutAndSSTableSize() {
+ try {
+ Map<String, String> options = new HashMap<>();
+ options.put("class", "LeveledCompactionStrategy");
+ options.put("fanout_size", "90");
+ options.put("sstable_size_in_mb", "1089");
+ LeveledCompactionStrategy.validateOptions(options);
+ }
+ catch (ConfigurationException e) {
+ assertTrue(e.getMessage().contains("your maxSSTableSize must be
absurdly high to compute"));
+ return;
+ }
+ Assert.fail("fanout_sizeed and sstable_size_in_mb are invalid, but did
not throw ConfigurationException");
Review Comment:
nit, if you move this into the `try` block you can remove the `return`
##########
test/unit/org/apache/cassandra/db/compaction/LeveledCompactionStrategyTest.java:
##########
@@ -969,6 +969,36 @@ public void testReduceScopeL0L1() throws IOException
}
}
+ @Test()
+ public void testInvalidFanoutAndSSTableSize() {
+ try {
+ Map<String, String> options = new HashMap<>();
+ options.put("class", "LeveledCompactionStrategy");
+ options.put("fanout_size", "90");
+ options.put("sstable_size_in_mb", "1089");
+ LeveledCompactionStrategy.validateOptions(options);
+ }
+ catch (ConfigurationException e) {
+ assertTrue(e.getMessage().contains("your maxSSTableSize must be
absurdly high to compute"));
+ return;
+ }
+ Assert.fail("fanout_sizeed and sstable_size_in_mb are invalid, but did
not throw ConfigurationException");
+ }
+
+ @Test
+ public void testValidOptions() {
+ try {
+ Map<String, String> options = new HashMap<>();
+ options.put("class", "LeveledCompactionStrategy");
+ options.put("fanout_size", "10");
+ options.put("sstable_size_in_mb", "160");
+ LeveledCompactionStrategy.validateOptions(options);
+ }
+ catch (Exception e) {
Review Comment:
style: move `{` to the next line
--
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]
