[
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):
{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"{}}}?
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
