[
https://issues.apache.org/jira/browse/CASSANDRA-9318?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15386265#comment-15386265
]
Sergio Bossa commented on CASSANDRA-9318:
-----------------------------------------
[~Stefania],
I've addressed most of your concerns in my latest two commits, also see my
following comments:
bq. If we removed MessagingService.getBackPressurePerHost(), which doesn't seem
to be used, then we can at least remove getBackPressureRateLimit and getHost
from BackPressureState, leaving it with only onMessageSent and
onMessageReceived.
All of those are used for JMX monitoring (which will be improved more) or
logging, so I'd keep them.
bq. We can also consider moving the states internally to the strategy
I think if we add too many responsibilities to the strategy it really starts to
look less and less as an actual strategy; we could rather add a
"BackPressureManager", but to be honest, the current responsibility assignment
feels natural to me, and back-pressure is naturally a "messaging layer"
concern, so I'd keep everything as it is unless you can show any actual
advantages in changing the design.
bq. We are also applying backpressure after sending the messages, is this
intentional?
This _was_ intentional, so that outgoing requests could have been processed
while pausing; but it had the drawback of making the rate limiting less
"predictable" (as dependant on which thread was going to be called next), so I
ended up changing it.
bq. In the very same comparator lambda, if 2 states are not equal but have the
exact same rate limit, then I assume it is intentional to compare them by hash
code in order to get a stable sort? If so we should add a comment since tree
set only requires the items to be mutually comparable and therefore returning
zero would have been allowed as well I believe.
It's not a matter of making the sort stable, but a matter of making the
comparator fully consistent with {{equals}}, which should be already documented
by the treeset/comparator javadoc.
bq. One failure that needs to be looked at:
Fixed!
bq. Let's test the strategy first, then metrics can be added later on.
Agreed. If we all (/cc [~iamaleksey] [~slebresne] [~jbellis]) agree on the core
concepts of the current implementation, I will rebase to trunk, remove the
trailing spaces, and then I would move into testing it, and in the meantime
think/work on adding metrics and making any further adjustments.
> Bound the number of in-flight requests at the coordinator
> ---------------------------------------------------------
>
> Key: CASSANDRA-9318
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9318
> Project: Cassandra
> Issue Type: Improvement
> Components: Local Write-Read Paths, Streaming and Messaging
> Reporter: Ariel Weisberg
> Assignee: Sergio Bossa
> Attachments: 9318-3.0-nits-trailing-spaces.patch, backpressure.png,
> limit.btm, no_backpressure.png
>
>
> It's possible to somewhat bound the amount of load accepted into the cluster
> by bounding the number of in-flight requests and request bytes.
> An implementation might do something like track the number of outstanding
> bytes and requests and if it reaches a high watermark disable read on client
> connections until it goes back below some low watermark.
> Need to make sure that disabling read on the client connection won't
> introduce other issues.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
