Page MenuHomePhabricator

Check whether User::pingLimiter can now use incr() and changeTTL() instead of the lock()/merge() workaround
Closed, ResolvedPublic

Description

/cc @aaron @daniel

During an unrelated conversation today, Aaron and I walked into this code and were wondering whether the overhead/workaround is still needed. This is a placeholder to look into that at some point.

Related context:

Event Timeline

Memcached alledgedly retaining values too long. I don't know if we've since confirmed this.

I forgot about T246991, but out of curiosity I've just went to logstash and searched for the debug message added in r622541. That case was triggered roughly 400 times in the last 15 minutes, which seems very wrong to me.

@daniel There is indeed of a fairly high and constant volumeof Logstash entries under channel:ratelimit (Logstash query), with the message "Stale rate limit entry, cache key failed to expire (T246991)".

The problem with merge() is that it will stall if the average interval between writes is less than the time taken by the mergeViaCas() loop body. Additional write traffic will keep failing to merge and after 10 attempts will return false, silently failing to increment the counter. For example in production we have a limit for edits by autopatrolled users of 10500/180 = 58 edits per second, which might be unreachable with this limiter implementation.

Putting the logic on the server side, as in that GitHub blog post, seems like a reasonable way to do it, but I don't know if it's fashionable to introduce new usages of Redis in production.