[
https://issues.apache.org/jira/browse/CASSANDRA-16959?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17415568#comment-17415568
]
Andres de la Peña commented on CASSANDRA-16959:
-----------------------------------------------
The proposed patch immediately applies the rate and considers negative values
as unlimited:
||PR||CI||
|[trunk|https://github.com/apache/cassandra/pull/1202]|[j8|https://app.circleci.com/pipelines/github/adelapena/cassandra/842/workflows/2f8cbef4-0e93-4079-ac77-ba7223c1ecff]
[j11|https://app.circleci.com/pipelines/github/adelapena/cassandra/842/workflows/41966994-0d46-4b57-a747-06a70b13f238]|
I've opted for accepting negative values for simplicity, just to not repeat the
validation for the value in the configuration and in the two nodetool commands.
We can easily reject negative values if we prefer so.
Also I wonder whether we should improve [the log
message|https://github.com/apache/cassandra/blob/09c89e5f5f8604301c233130dfb6e82a36ae30f3/src/java/org/apache/cassandra/service/StorageService.java#L1489]
to include things like the unit of measurement, the previous rate or if the
value is considered unlimited. My only concern about this is compatibility, in
case some tooling out there depends on these log messages.
All praise to Niteshwar Kumar, who originally wrote the patch. The tiny tests
for {{nodetool setstreamthroughput/setinterdcstreamthroughput}} are mine.
> nodetool setstreamthroughput accepts invalid arguments that are not
> immediately applied
> ---------------------------------------------------------------------------------------
>
> Key: CASSANDRA-16959
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16959
> Project: Cassandra
> Issue Type: Bug
> Components: Tool/nodetool
> Reporter: Andres de la Peña
> Assignee: Andres de la Peña
> Priority: Normal
>
> Both {{nodetool setstreamthroughput}} and {{nodetool
> setinterdcstreamthroughput}} accept a negative throughput. The throughput
> value is not immediately applied to the corresponding rate limiters. Instead,
> the value is [set in the
> {{Config}}|https://github.com/apache/cassandra/blob/09c89e5f5f8604301c233130dfb6e82a36ae30f3/src/java/org/apache/cassandra/service/StorageService.java#L1488]
> and it's only applied to the singleton rate limiter when new sstable stream
> writer are created (see
> [here|https://github.com/apache/cassandra/blob/09c89e5f5f8604301c233130dfb6e82a36ae30f3/src/java/org/apache/cassandra/streaming/StreamManager.java#L66-L76]).
> This could happen much later than the definition of the new throughput, and
> by then the setting of the new rate in the rate limiter will fail with an
> {{IllegalArgumentException}} due to the negative value.
> I think we should either immediately reject negative throughputs or consider
> them unlimited, as we do with zero. Also we should probably apply the new
> throughput to the rate limiter immediately, since I don't see why we should
> wait to start using the new throughput.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
