You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "Caleb Rackliffe (Jira)" <ji...@apache.org> on 2022/07/07 21:25:00 UTC
[jira] [Comment Edited] (CASSANDRA-17725) Add a flag for throughput in MiB/s for nodetool setstreamthroughput, getstreamthroughput, setinterdcstreamthroughput and getinterdcstreamthroughput
[ https://issues.apache.org/jira/browse/CASSANDRA-17725?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17563982#comment-17563982 ]
Caleb Rackliffe edited comment on CASSANDRA-17725 at 7/7/22 9:24 PM:
---------------------------------------------------------------------
{quote}The only thing I am wondering here is how to handle that nodetool and the StorageService JMX methods are rounding to int and 0 very small values in megabits to MiB/s (the getters in MiB, when we provide too small megabits) I added in nodetool a check that will say instead of unlimited - 1 MiB/s. I plan to document that when you use one or the other unit you should use the respective setter/getter as otherwise you might see that side effect. Seeing 0 in MiB - someone might decide that they are unthrottling when in reality they have super low value in megabits internally. What do others think about this?
{quote}
Why don't we just simplify things and leave MiB/s values as doubles everywhere? If we do that for JMX and {{{}nodetool{}}}, can't we just sidestep the rounding ambiguity almost entirely? (I mean, we still need to decide on how many decimals we'll report, but we can probably do 2 or 3 and call it a day.) I don't see any reason to force a rounded/integer output when it would only materialize in conjunction with our new flag.
Other minor notes from my pass at review:
- In the {{MEGABITS_TO_MEBIBYTES_PER_SECOND_DATA_RATE}} JavaDoc, "megatibs" -> "megabits"?
- There's enough in common that it might be nice to combine {{SetGetInterDCStreamThroughputMiBTest}} and {{SetGetInterDCStreamThroughputTest}} into a single {{{}InterDCStreamThroughputConfigTest{}}}. (Same goes for the non-inter-DC versions.)
- In {{{}StorageService{}}}, we have {{{}int oldValue = (int) DatabaseDescriptor.getStreamThroughputOutboundMebibytesPerSec();{}}}. Why not just get the existing value as a double and log it that way? (Happens in {{setStreamThroughputMebibytesPerSec}} and {{{}setInterDCStreamThroughputMebibytesPerSec{}}}.)
- Given we're going to a new major version, did we really have to change {{MiB}} back to {{MB}} in [this commit|https://github.com/ekaterinadimitrova2/cassandra/commit/92e0c3948fc65b29fbe289814f84aaf603188152]? Even if someone was parsing tool output, they still would only break if they explicitly validated "MB" rather than just making sure to take the numeric value, right?
- Two things on {{{}"-i", "--stream_throughput_mib"{}}}. First, is the normal pattern to use hyphens instead of underscores for the long form? Second, given this is a modifier on the default argument, why not just use something like {{{}"-m", "--mib"{}}}?
was (Author: maedhroz):
bq. The only thing I am wondering here is how to handle that nodetool and the StorageService JMX methods are rounding to int and 0 very small values in megabits to MiB/s (the getters in MiB, when we provide too small megabits) I added in nodetool a check that will say instead of unlimited - 1 MiB/s. I plan to document that when you use one or the other unit you should use the respective setter/getter as otherwise you might see that side effect. Seeing 0 in MiB - someone might decide that they are unthrottling when in reality they have super low value in megabits internally. What do others think about this?
Why don't we just simplify things and leave MiB/s values as doubles everywhere? If we do that for JMX and {{nodetool}}, can't we just sidestep the rounding ambiguity almost entirely? (I mean, we still need to decide on how many decimals we'll report, but we can probably do 2 or 3 and call it a day.) I don't see any reason to force a rounded/integer output when it would only materialize in conjunction with our new flag.
Other minor notes from my pass at review:
- In the {{MEGABITS_TO_MEBIBYTES_PER_SECOND_DATA_RATE}} JavaDoc, "megatibs" -> "megabits"?
- There's enough in common that it might be nice to combine {{SetGetInterDCStreamThroughputMiBTest}} and {{SetGetInterDCStreamThroughputTest}} into a single {{InterDCStreamThroughputConfigTest}}. (Same goes for the non-inter-DC versions.)
- In {{StorageService}}, we have {{int oldValue = (int) DatabaseDescriptor.getStreamThroughputOutboundMebibytesPerSec();}}. Why not just get the existing value as a double and log it that way? (Happens in {{setStreamThroughputMebibytesPerSec}} and {{setInterDCStreamThroughputMebibytesPerSec}}.)
- Given we're going to a new major version, did we really have to change {{MiB}} back to {{MB}} in [this commit|https://github.com/ekaterinadimitrova2/cassandra/commit/92e0c3948fc65b29fbe289814f84aaf603188152]? Even if someone was parsing tool output, they still would only break if they explicitly validated "MB" rather than just making sure to take the numeric value, right?
- Two things on {{"-i", "--stream_throughput_mib"}}. First, is the normal pattern to use hyphens instead of underscores for the long form? Second, given this is a modifier on the default argument, why not just use something like {{"-m", "--mib"}}?
> Add a flag for throughput in MiB/s for nodetool setstreamthroughput, getstreamthroughput, setinterdcstreamthroughput and getinterdcstreamthroughput
> ----------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: CASSANDRA-17725
> URL: https://issues.apache.org/jira/browse/CASSANDRA-17725
> Project: Cassandra
> Issue Type: Bug
> Components: Tool/nodetool
> Reporter: Ekaterina Dimitrova
> Assignee: Ekaterina Dimitrova
> Priority: Normal
> Fix For: 4.1-beta, 4.x
>
>
> As we agreed not to add new JMX methods for the new config on the mailing list, we need at least new flags for setstreamthroughput and interdcstreamthroughput for the two 4.0 parameters to be set/get also in MiB, not only in megabits.
> Thus we will have the option either to use the old version for those 2, or to be able to set/get in MiB all 4 streaming parameters. As of 4.1 supported units for DataRateSpec are MiB/s, B/s, KiB/s, megabit is only for legacy from 4.0 - backward compatibility.
> To be sure we satisfy the requirements around the latest discussions about backward compatibility in tools, I will use this ticket also to make a final pass on the unit changes done, to ensure the probe output is not affected.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org