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/01 22:54:59 UTC
[GitHub] [cassandra] frankgh opened a new pull request, #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
frankgh opened a new pull request, #1658:
URL: https://github.com/apache/cassandra/pull/1658
`entire_sstable_stream_throughput_outbound` and `entire_sstable_inter_dc_stream_throughput_outbound` were
introduced in CASSANDRA-17065. They were added to the LoaderOptions class but they are not being used in
the BulkLoader class. In this commit, we make use of the entire_sstable streaming options in BulkLoader.
In addittion, the `throttle` and `interDcThrottle` are being passed as MB/s but they should be MiB/s.
This commit fixes the verbiage as well as setting the correct value for those properties in BulkLoader.
--
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
[GitHub] [cassandra] frankgh commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r899222631
##########
doc/modules/cassandra/pages/operating/bulk_loading.adoc:
##########
@@ -80,13 +80,20 @@ The following options are supported, with `-d,--nodes <initial hosts>` required:
-d,--nodes <initial hosts> Required.
Try to connect to these hosts (comma separated) initially for ring information
+-e,--entire-sstable-throttle <throttle> Entire SSTable throttle
+ speed in MiB/s (default 24MiB/s).
Review Comment:
yes, this is correct for the bulk loading. I will adjust it
--
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
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r901881363
##########
doc/modules/cassandra/pages/operating/bulk_loading.adoc:
##########
@@ -80,13 +80,20 @@ The following options are supported, with `-d,--nodes <initial hosts>` required:
-d,--nodes <initial hosts> Required.
Try to connect to these hosts (comma separated) initially for ring information
+-e,--entire-sstable-throttle <throttle> Entire SSTable throttle
+ speed in MiB/s (default 24MiB/s).
Review Comment:
Agreed, the change was triggered by the new doc and not the opposite - those were unlimited in the code for the tool for the 4 options
--
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
[GitHub] [cassandra] frankgh commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r899301668
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
+ options.addOption("idct", INTER_DC_THROTTLE_MEGABITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbps (default unlimited)");
+ options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MEBIBYTES, "entire-sstable-throttle", "entire SSTable throttle speed in MiB/s (default unlimited)");
+ options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MEBIBYTES, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in MiB/s (default unlimited)");
Review Comment:
what do you think @ekaterinadimitrova2 about Yifan's proposal? my only concern with this approach is that we need to worry about the case where the user provides both parameters with different units. So we'll need to address that case
--
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
[GitHub] [cassandra] frankgh commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r902713423
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
+ options.addOption("idct", INTER_DC_THROTTLE_MEGABITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbps (default unlimited)");
+ options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MEBIBYTES, "entire-sstable-throttle", "entire SSTable throttle speed in MiB/s (default unlimited)");
+ options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MEBIBYTES, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in MiB/s (default unlimited)");
Review Comment:
So with Yifan's suggestion we'll have the following:
`t`, `throttle` -> megabits (deprecated)
`tmib`, `throttle-mib` -> mebibytes (new)
`emib`, `entire-sstable-throttle-mib` -> mebibytes (new)
`idct`, `inter-dc-throttle` -> megabits (deprecated)
`idctmib`, `inter-dc-throttle-mib` -> mebibytes (new)
`eidctmib`, `entire-sstable-inter-dc-throttle-mib` -> mebibytes (new)
What do you think @yifan-c, @ekaterinadimitrova2 ?
--
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
[GitHub] [cassandra] frankgh commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r904110789
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
+ options.addOption("idct", INTER_DC_THROTTLE_MEGABITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbps (default unlimited)");
+ options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MEBIBYTES, "entire-sstable-throttle", "entire SSTable throttle speed in MiB/s (default unlimited)");
+ options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MEBIBYTES, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in MiB/s (default unlimited)");
Review Comment:
I pushed the changes above. Let me know what you think
--
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
[GitHub] [cassandra] yifan-c commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
yifan-c commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r897338591
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
+ options.addOption("idct", INTER_DC_THROTTLE_MEGABITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbps (default unlimited)");
+ options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MEBIBYTES, "entire-sstable-throttle", "entire SSTable throttle speed in MiB/s (default unlimited)");
+ options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MEBIBYTES, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in MiB/s (default unlimited)");
Review Comment:
In this approach, would it make sense for all four options has the same unit as MEGABITS, or declare new names for the ones using MEBIBYTES? I'd prefer the later one.
The deprecation approach requires to declare _new_ options and mark the old ones as deprecated. Both new and old options are present. Currently, the option names are consistent with different unit. When the new option is added, there will be options names that are not consistent, but the unit is. Let's say, the new option name for throttle are `tmib` and `throttle-mib` (shorthand and full accordingly). `tmib` and `e` has inconsistent name, but they use the same unit.
--
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
[GitHub] [cassandra] yifan-c commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
yifan-c commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r897331164
##########
doc/modules/cassandra/pages/operating/bulk_loading.adoc:
##########
@@ -80,13 +80,20 @@ The following options are supported, with `-d,--nodes <initial hosts>` required:
-d,--nodes <initial hosts> Required.
Try to connect to these hosts (comma separated) initially for ring information
+-e,--entire-sstable-throttle <throttle> Entire SSTable throttle
+ speed in MiB/s (default 24MiB/s).
Review Comment:
Talked offline. Although the document says 24 MiB/s, the default is actually unlimited. I feel it makes sense with unlimited for the tool
--
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
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r902723155
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
Review Comment:
SGTM
--
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
[GitHub] [cassandra] frankgh commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r899235072
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
Review Comment:
I agree that we should keep the `unlimited` as default since it's an external tool.
--
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
[GitHub] [cassandra] frankgh commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r902605390
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
Review Comment:
maybe the text should read `(default 0 for unlimited)`
--
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
[GitHub] [cassandra] smiklosovic closed pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
URL: https://github.com/apache/cassandra/pull/1658
--
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
[GitHub] [cassandra] yifan-c commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
yifan-c commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r917143009
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -60,10 +72,14 @@
public static final String IGNORE_NODES_OPTION = "ignore";
public static final String CONNECTIONS_PER_HOST = "connections-per-host";
public static final String CONFIG_PATH = "conf-path";
- public static final String THROTTLE_MBITS = "throttle";
- public static final String INTER_DC_THROTTLE_MBITS = "inter-dc-throttle";
- public static final String ENTIRE_SSTABLE_THROTTLE_MBITS = "entire-sstable-throttle";
- public static final String ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS = "entire-sstable-inter-dc-throttle";
+ @Deprecated
+ public static final String THROTTLE_MEGABITS = "throttle";
+ public static final String THROTTLE_MEBIBYTES = "throttle-mib";
+ @Deprecated
+ public static final String INTER_DC_THROTTLE_MEGABITS = "inter-dc-throttle";
+ public static final String INTER_DC_THROTTLE_MEBIBYTES = "inter-dc-throttle-mib";
+ public static final String ENTIRE_SSTABLE_THROTTLE_MEBIBYTES = "entire-sstable-throttle-mib";
+ public static final String ENTIRE_SSTABLE_INTER_DC_THROTTLE_MEBIBYTES = "entire-sstable-inter-dc-throttle-mib";
Review Comment:
@frankgh, can you also revert the renaming in this PR? THX
--
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
[GitHub] [cassandra] yifan-c commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
yifan-c commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r897330131
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
Review Comment:
If you are updating the default throttling, also update here. However, I'd prefer to keep it the same (unlimited), since it is an external tool.
--
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
[GitHub] [cassandra] yifan-c commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
yifan-c commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r903294436
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
+ options.addOption("idct", INTER_DC_THROTTLE_MEGABITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbps (default unlimited)");
+ options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MEBIBYTES, "entire-sstable-throttle", "entire SSTable throttle speed in MiB/s (default unlimited)");
+ options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MEBIBYTES, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in MiB/s (default unlimited)");
Review Comment:
Looks good to me.
--
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
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r904103152
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
+ options.addOption("idct", INTER_DC_THROTTLE_MEGABITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbps (default unlimited)");
+ options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MEBIBYTES, "entire-sstable-throttle", "entire SSTable throttle speed in MiB/s (default unlimited)");
+ options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MEBIBYTES, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in MiB/s (default unlimited)");
Review Comment:
`idctib` and `eidctmib` sound a bit long but they follow the already established naming convention and when people know where it comes from I guess it will be intuitive.
--
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
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r888386976
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
+ options.addOption("idct", INTER_DC_THROTTLE_MEGABITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbps (default unlimited)");
+ options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MEBIBYTES, "entire-sstable-throttle", "entire SSTable throttle speed in MiB/s (default unlimited)");
+ options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MEBIBYTES, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in MiB/s (default unlimited)");
Review Comment:
Note for the second reviewer: we were discussing with Francisco that it would have been nice if we had all 4 parameters with the same unit. Probably we can add a note that those in bits are deprecated(but still keep them for compatibility) and add two more for them which can be used for the mebibytes/s. On the other hand we are already in a freeze so probably the best is to leave things as is now and add for the next release in trunk directly the opportunity to set them with units, similar to the cassandra.yaml
--
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
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r914082206
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -85,10 +101,10 @@
public final String user;
public final String passwd;
public final AuthProvider authProvider;
- public final int throttle;
- public final int interDcThrottle;
- public final int entireSSTableThrottle;
- public final int entireSSTableInterDcThrottle;
+ public final double throttleMegabits;
+ public final double interDcThrottleMegabits;
Review Comment:
@yifan-c , @frankgh considering our later discussions and the ticket that @yifan-c pointed to earlier today, probably we should find a way to workaround this change too? WDYT?
--
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
[GitHub] [cassandra] yifan-c commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
yifan-c commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r917166505
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -85,10 +101,10 @@
public final String user;
public final String passwd;
public final AuthProvider authProvider;
- public final int throttle;
- public final int interDcThrottle;
- public final int entireSSTableThrottle;
- public final int entireSSTableInterDcThrottle;
+ public final double throttleMegabits;
+ public final double interDcThrottleMegabits;
Review Comment:
I feel less worried about someone reading the field values... I can only image the use cases that use the option name constants to invoke the tool, or construct the option programmatically. The change does not affect those use cases.
--
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
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r901892637
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
Review Comment:
By the way I noticed we mention unlimited but it is not explicitly said that to make it unlimited you need to set it to 0. I checked also in cassandra.yaml and I didn't find that. Maybe we should document it?
--
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
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1658: CASSANDRA-17677: Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1658:
URL: https://github.com/apache/cassandra/pull/1658#discussion_r901876049
##########
src/java/org/apache/cassandra/tools/LoaderOptions.java:
##########
@@ -653,10 +653,10 @@ private static CmdLineOptions getCmdLineOptions()
options.addOption("p", NATIVE_PORT_OPTION, "native transport port", "port used for native connection (default 9042)");
options.addOption("sp", STORAGE_PORT_OPTION, "storage port", "port used for internode communication (default 7000)");
options.addOption("ssp", SSL_STORAGE_PORT_OPTION, "ssl storage port", "port used for TLS internode communication (default 7001)");
- options.addOption("t", THROTTLE_MBITS, "throttle", "throttle speed in Mbits (default unlimited)");
- options.addOption("idct", INTER_DC_THROTTLE_MBITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbits (default unlimited)");
- options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MBITS, "entire-sstable-throttle", "entire SSTable throttle speed in Mbits (default unlimited)");
- options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MBITS, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in Mbits (default unlimited)");
+ options.addOption("t", THROTTLE_MEGABITS, "throttle", "throttle speed in Mbps (default unlimited)");
+ options.addOption("idct", INTER_DC_THROTTLE_MEGABITS, "inter-dc-throttle", "inter-datacenter throttle speed in Mbps (default unlimited)");
+ options.addOption("e", ENTIRE_SSTABLE_THROTTLE_MEBIBYTES, "entire-sstable-throttle", "entire SSTable throttle speed in MiB/s (default unlimited)");
+ options.addOption("eidct", ENTIRE_SSTABLE_INTER_DC_THROTTLE_MEBIBYTES, "entire-sstable-inter-dc-throttle", "entire SSTable inter-datacenter throttle speed in MiB/s (default unlimited)");
Review Comment:
The deprecation approach suggested by @yifan-c makes sense to me.
I would expect if someone wants to use the 4 of them to use the new ones mebibytes/s for all of them.
The old two properties with megabits/s are for compatibility with scripts. If someone wants to add also the other 2 they will decide whether they want to migrate the old ones to the new units or just add the two new properties with different units and be inconsistent.
--
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