[
https://issues.apache.org/jira/browse/CASSANDRA-11303?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15259360#comment-15259360
]
Paulo Motta commented on CASSANDRA-11303:
-----------------------------------------
Thanks for the patch [~skonno]. Overall the approach looks good. See comments
and suggestions for improvement below:
* While we can do rate limiting directly at {{StreamReader}}, on
{{CompressedStreamReader}} the actual read from the network is performed by
{{CompressedInputStream.Reader}}, and read bytes are uncompressed and cached
for consumption on {{CompressedStreamReader}} so we probably need to do rate
limiting on {{CompressedInputStream.Reader}} instead.
* We will need to update {{StorageServiceMBean}} to add new methods to get and
set inbound throughput in real time via JMX or {{nodetool
(get/set)streamthroughput}}
** For that, we need to deprecate existing methods
{{(set/get)StreamThroughputMbPerSec}} and
{{(set/get)InterDCStreamThroughputMbPerSec}} (we can't remove them yet), and
add new methods: {{(set/get)(Inbound/Outbound)StreamThroughputMbPerSec}} and
{{(set/get)(Inbound/Outbound)InterDCStreamThroughputMbPerSec}}
* After that, we will need to add new parameters to {{nodetool
setstreamthroughput}} and {{nodetool getstreamthroughput}}, perhaps something
like: {{nodetool setstreamthroughput --inbound 10}} and {{nodetool
getstreamthroughput --outbound}}, and maybe get/set outbound by default if
parameters are not specified to maintain backward compatibility?
* The same static {{RateLimiter}} object is being shared between inbound and
outgoing streams on {{StreamRateLimiter}}, which means the rate will be divided
between inbound and outbound streams. What we actually want is to have
independent inbound and outbound throughput limiting, so you'll need to have 4
static RateLimiter objects: {{inboundLimiter}}, {{outboundLimiter}},
{{inboundInterDCLimiter}}, {{outboundInterDCLimiter}}.
** We also need to update them when
{{StorageServiceMBean.set(Inbound/Outbound)(InterDC)StreamThroughputMbPerSec}}
is called.
* I think we can move static factory methods {{getInboundRateLimiter}} and
{{getOutboundRateLimiter}} from {{StreamManager}} to {{StreamRaterLimiter}}
* We're creating a {{StreamInboundRateLimiter}} per peer per stream session,
but we actually need only 2 {{StreamInboundRateLimiter}} instances: a local-dc
and an inter-dc
** The factory method {{getInboundRateLimiter(peer)}} can decide which instance
to pick if peer is from local or remote dc
* I don't think we need separate {{StreamInboundRateLimiter}} and
{{StreamOutboundRateLimiter}} classes since they're essentially the same,
except for their underlying rate limiters which can be set by their factory
methods
* Since we will already update the limiters when
{{StorageServiceMBean.set(Inbound/Outbound)(InterDC)StreamThroughputMbPerSec}}
is called, we no longer need to call {{mayUpdateThroughput}} when fetching them
via {{getInboundRateLimiter}}, etc..
* Other comments
** Is there any particular reason you made {{StreamDeserializer}} class and
fields non-static?
** I noticed you changed a stream throughput comment on {{cassandra.yaml}} from
{{When unset, the default is 200 Mbps or 25 MB/s}} to {{When unset, the default
is no limit.}}, but this is not correct as the default values on {{Config}} if
these properties are unset on {{cassandra.yaml}} is in fact 200Mbps.
> New inbound throughput parameters for streaming
> -----------------------------------------------
>
> Key: CASSANDRA-11303
> URL: https://issues.apache.org/jira/browse/CASSANDRA-11303
> Project: Cassandra
> Issue Type: New Feature
> Components: Configuration
> Reporter: Satoshi Konno
> Priority: Minor
> Attachments: 11303_inbound_limit_debug_20160419.log,
> 11303_inbound_nolimit_debug_20160419.log,
> 11303_inbound_patch_for_trunk_20160419.diff, cassandra_inbound_stream.diff
>
>
> Hi,
> To specify stream throughputs of a node more clearly, I would like to add the
> following new inbound parameters like existing outbound parameters in the
> cassandra.yaml.
> - stream_throughput_inbound_megabits_per_sec
> - inter_dc_stream_throughput_outbound_megabits_per_sec
> We use only the existing outbound parameters now, but it is difficult to
> control the total throughputs of a node. In our production network, some
> critical alerts occurs when a node exceed the specified total throughput
> which is the sum of the input and output throughputs.
> In our operation of Cassandra, the alerts occurs during the bootstrap or
> repair processing when a new node is added. In the worst case, we have to
> stop the operation of the exceed node.
> I have attached the patch under consideration. I would like to add a new
> limiter class, StreamInboundRateLimiter, and use the limiter class in
> StreamDeserializer class. I use Row::dataSize( ）to get the input throughput
> in StreamDeserializer::newPartition(), but I am not sure whether the
> dataSize() returns the correct data size.
> Can someone please tell me how to do it ?
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
