You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/06/23 18:36:48 UTC
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1701: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
ekaterinadimitrova2 commented on code in PR #1701:
URL: https://github.com/apache/cassandra/pull/1701#discussion_r905265410
##########
test/unit/org/apache/cassandra/tools/LoaderOptionsTest.java:
##########
@@ -84,38 +84,158 @@ public void testEncryptionSettings() throws Exception
assertEquals("test.jks", options.clientEncOptions.keystore);
}
+ @Test
+ public void testThrottleDefaultSettings()
+ {
+ LoaderOptions options = LoaderOptions.builder().build();
+ assertEquals(0, options.throttleMebibytes);
+ assertEquals(0, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testDeprecatedThrottleSettings() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "-t", "200", "-idct", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
+ assertEquals(24, options.throttleMebibytes);
+ assertEquals(48, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testDeprecatedThrottleSettingsWithLongSettingNames() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "--throttle", "200", "--inter-dc-throttle", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
+ assertEquals(24, options.throttleMebibytes);
+ assertEquals(48, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testThrottleSettings() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "-tmib", "200", "-idctmib", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
Review Comment:
```suggestion
// converts from megabits to mebibytes
```
##########
test/unit/org/apache/cassandra/tools/LoaderOptionsTest.java:
##########
@@ -84,38 +84,158 @@ public void testEncryptionSettings() throws Exception
assertEquals("test.jks", options.clientEncOptions.keystore);
}
+ @Test
+ public void testThrottleDefaultSettings()
+ {
+ LoaderOptions options = LoaderOptions.builder().build();
+ assertEquals(0, options.throttleMebibytes);
+ assertEquals(0, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testDeprecatedThrottleSettings() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "-t", "200", "-idct", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
+ assertEquals(24, options.throttleMebibytes);
+ assertEquals(48, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testDeprecatedThrottleSettingsWithLongSettingNames() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "--throttle", "200", "--inter-dc-throttle", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
Review Comment:
```suggestion
// converts from megabits to mebibytes
```
##########
test/unit/org/apache/cassandra/tools/LoaderOptionsTest.java:
##########
@@ -84,38 +84,158 @@ public void testEncryptionSettings() throws Exception
assertEquals("test.jks", options.clientEncOptions.keystore);
}
+ @Test
+ public void testThrottleDefaultSettings()
+ {
+ LoaderOptions options = LoaderOptions.builder().build();
+ assertEquals(0, options.throttleMebibytes);
+ assertEquals(0, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testDeprecatedThrottleSettings() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "-t", "200", "-idct", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
+ assertEquals(24, options.throttleMebibytes);
+ assertEquals(48, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testDeprecatedThrottleSettingsWithLongSettingNames() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "--throttle", "200", "--inter-dc-throttle", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
+ assertEquals(24, options.throttleMebibytes);
+ assertEquals(48, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testThrottleSettings() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "-tmib", "200", "-idctmib", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
+ assertEquals(200, options.throttleMebibytes);
+ assertEquals(400, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testThrottleSettingsWithLongSettingNames() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "--throttle-mib", "200", "--inter-dc-throttle-mib", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
Review Comment:
```suggestion
// converts from megabits to mebibytes
```
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -494,24 +508,46 @@ public Builder parseArgs(String cmdArgs[])
System.exit(1);
}
- if (cmd.hasOption(THROTTLE_MBITS))
+ if (cmd.hasOption(THROTTLE_MEGABITS) && cmd.hasOption(THROTTLE_MEBIBYTES))
+ {
+ errorMsg(String.format("Both '%s' and '%s' were provided. Please only provide one of the two options", THROTTLE_MEGABITS, THROTTLE_MEBIBYTES), options);
+ }
+
+ if (cmd.hasOption(INTER_DC_THROTTLE_MEGABITS) && cmd.hasOption(INTER_DC_THROTTLE_MEBIBYTES))
+ {
+ errorMsg(String.format("Both '%s' and '%s' were provided. Please only provide one of the two options", INTER_DC_THROTTLE_MEGABITS, INTER_DC_THROTTLE_MEBIBYTES), options);
+ }
+
+ if (cmd.hasOption(THROTTLE_MEGABITS))
+ {
+ throttleMebibytes = DataRateSpec.IntMebibytesPerSecondBound.megabitsPerSecondInMebibytesPerSecond(Integer.parseInt(cmd.getOptionValue(THROTTLE_MEGABITS)))
+ .toMebibytesPerSecondAsInt();
Review Comment:
I think that this part has an issue and breaks the compatibility. The conversions between megabits and mebibytes are double numbers internally. Something very annoying to keep backward compatibility, etc.
So the issue with this approach here is that someone will set megabits but by using `.toMebibytesPerSecondAsInt()` to set the throttling we actually round the number. This can be caught if you try to add `--throttle` being `5megabits` in `testBulkLoader_WithArgs6`. Then if you do `assertEquals(5, DatabaseDescriptor.getStreamThroughputOutboundMegabitsPerSec());` you actually get 8.
As it is a tool and the difference doesn't seem super big might be ok.
The bad part is that if someone tries super small number just for testing or something, they will actually get 0 and unthrottle... That is how I identified that annoying issue when we worked on the new config framework and we switched to `double` in DataRateSpec to be precise as the `RateLimiter` gets double anyway. At the same time, of course, we do not allow the users to input `double` numbers, just `int`. The `SetGetStreamThroughputTest` tests small numbers but we didn't have this for the tool and it is very tiny detail easy to miss. :-(
I am thinking maybe we can introduce double variable to ensure precision and no surprises to people who continue using the megabits version? WDYT?
In general probably we can just document for people this behavior and leave it as is I guess but if we want to keep the compatibility and be precise everywhere, we will need to introduce `double`. I guess this shouldn't be hard so at this point of the release I guess I would prefer just to work our the precision to be correct.
CC @yifan-c
##########
test/unit/org/apache/cassandra/tools/LoaderOptionsTest.java:
##########
@@ -84,38 +84,158 @@ public void testEncryptionSettings() throws Exception
assertEquals("test.jks", options.clientEncOptions.keystore);
}
+ @Test
+ public void testThrottleDefaultSettings()
+ {
+ LoaderOptions options = LoaderOptions.builder().build();
+ assertEquals(0, options.throttleMebibytes);
+ assertEquals(0, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testDeprecatedThrottleSettings() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "-t", "200", "-idct", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
+ assertEquals(24, options.throttleMebibytes);
+ assertEquals(48, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testDeprecatedThrottleSettingsWithLongSettingNames() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "--throttle", "200", "--inter-dc-throttle", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
+ assertEquals(24, options.throttleMebibytes);
+ assertEquals(48, options.interDcThrottleMebibytes);
Review Comment:
After we fix the precision issue, I think it will be correct to check the megabits here
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -494,24 +508,46 @@ public Builder parseArgs(String cmdArgs[])
System.exit(1);
}
- if (cmd.hasOption(THROTTLE_MBITS))
+ if (cmd.hasOption(THROTTLE_MEGABITS) && cmd.hasOption(THROTTLE_MEBIBYTES))
+ {
+ errorMsg(String.format("Both '%s' and '%s' were provided. Please only provide one of the two options", THROTTLE_MEGABITS, THROTTLE_MEBIBYTES), options);
+ }
+
+ if (cmd.hasOption(INTER_DC_THROTTLE_MEGABITS) && cmd.hasOption(INTER_DC_THROTTLE_MEBIBYTES))
+ {
+ errorMsg(String.format("Both '%s' and '%s' were provided. Please only provide one of the two options", INTER_DC_THROTTLE_MEGABITS, INTER_DC_THROTTLE_MEBIBYTES), options);
+ }
Review Comment:
Good call! :-)
##########
test/unit/org/apache/cassandra/tools/LoaderOptionsTest.java:
##########
@@ -84,38 +84,158 @@ public void testEncryptionSettings() throws Exception
assertEquals("test.jks", options.clientEncOptions.keystore);
}
+ @Test
+ public void testThrottleDefaultSettings()
+ {
+ LoaderOptions options = LoaderOptions.builder().build();
+ assertEquals(0, options.throttleMebibytes);
+ assertEquals(0, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testDeprecatedThrottleSettings() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "-t", "200", "-idct", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
Review Comment:
```suggestion
// converts from megabits to mebibytes
```
##########
test/unit/org/apache/cassandra/tools/LoaderOptionsTest.java:
##########
@@ -84,38 +84,158 @@ public void testEncryptionSettings() throws Exception
assertEquals("test.jks", options.clientEncOptions.keystore);
}
+ @Test
+ public void testThrottleDefaultSettings()
+ {
+ LoaderOptions options = LoaderOptions.builder().build();
+ assertEquals(0, options.throttleMebibytes);
+ assertEquals(0, options.interDcThrottleMebibytes);
+ }
+
+ @Test
+ public void testDeprecatedThrottleSettings() throws IOException
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf", "cassandra.yaml").normalize());
+ String[] args = { "-t", "200", "-idct", "400", "-d", "127.9.9.1", "-f", config.absolutePath(), sstableDirName("legacy_sstables", "legacy_ma_simple") };
+ LoaderOptions options = LoaderOptions.builder().parseArgs(args).build();
+ // converts from megabytes to mebibytes
+ assertEquals(24, options.throttleMebibytes);
+ assertEquals(48, options.interDcThrottleMebibytes);
+ }
Review Comment:
After we fix the precision issue, I think it will be correct to check the megabits here
##########
test/unit/org/apache/cassandra/tools/LoaderOptionsTest.java:
##########
@@ -84,38 +84,158 @@ public void testEncryptionSettings() throws Exception
assertEquals("test.jks", options.clientEncOptions.keystore);
}
+ @Test
+ public void testThrottleDefaultSettings()
+ {
+ LoaderOptions options = LoaderOptions.builder().build();
+ assertEquals(0, options.throttleMebibytes);
+ assertEquals(0, options.interDcThrottleMebibytes);
Review Comment:
nit: shall we add assertions also for the other two?
--
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