Regarding RedisLockManager (it only needs 2 of the 3 host to be reachable). If one of them is depooled or refuses connections, no one should notice any disruption. For otherwise unreachable servers, there is a 2 second timeout (and the redis server will be avoided for the rest of the request).

Added cache hit latency panel

It would be good to get cleanup patches like https://gerrit.wikimedia.org/r/c/mediawiki/core/+/596082 merged first.

The default timeout for waitForReplication() for web requests is 1 second. JobRunner passes 3 seconds to its calls and sets setDefaultReplicationWaitTimeout( 3 ). Maybe TranslateRenderJob should use a different timeout?

This only happens with paratest, so I suspect that some tests depend on each other running by accident.

Are the jobs failing near the end and getting retried? Also, job B can still be enqueued if a duplicate job A is marked as running. This was long-since the semantic logic (and the kafka queue does this too last I checked). If job A fails, it seems like the redis job queue (https://github.com/wikimedia/mediawiki-services-jobrunner/blob/master/redisJobChronService) would then allow duplicate job C, though JobQueueDB would not. Not sure about the Kafka queue in that situation. The behavior there should be standardized in any case.

The Grafana dashboards will still need updating.

The problem with LocalClusterCache is that there can be cycles or odd behavior for early calls. As long as the wiring works, then I'd love to have such a method.

Declined, assuming that the DB main stash has enough GB of space.

It's not meaningfully used, though it is NON NULL so the code still references it during insertion (using -1).

In T259520#6375101, @Marostegui wrote:

1k/min would be considered high for codfw I would say, eqiad is a different story.
What looks strange to me is the fact that I stopped centralauth master and the wiki that showed up as lagged was enwiki. Although on this last event, stopping wikidata's master, only shows wikidata as lagged, which is what I would have expected with s7 (where centralauth lives).

By "showed up as lagged", do you mean that there were mosly logstash messages with wiki:enwiki or did you see this in some graph of MW statsd data or such? I could probably tweak the "all replica DBs lagged" entries to have some extra fields.

In T254430#6460367, @daniel wrote:

Analysis:

LoadBalancer::isMasterConnectionReadOnly relies on caching in $this->srvCache. srvCache defaults to EmptyBagOStuff, which would explain why isMasterConnectionReadOnly hits serverIsReadOnly every time. However, LoadBalancer doesn't use the default value for srvCache, since LBFactory actually provides that values. It in turn gets it from ServiceWiring via MWLBFactory. ServiceWiring gets it from ObjectCache::makeLocalServerCache(). But ObjectCache::makeLocalServerCache() will return an EmptyBagOStuff in CLI mode.

I suppose that means that this isn't a real problem for us in production, since we call the job runner via the web. But for people who use maintenance/runJobs.php, it would be a problem.

In general, it seems strange that ObjectCache::makeLocalServerCache() wouldn't just return a HashBagOStuff. Code that wants a "local server cache" would probably work better with a cache that is transient, than with no cache at all. Is there a good reason not to have makeLocalServerCache() return a HashBagOStuff as a fallback?

In T221159#5562483, @daniel wrote:

Patch was merged, removing the patch for review tag.

Pn the patch, @jcrespo said:

I believe this is not working as intended, not because code- I can see now SELECT MASTER_GTID_WAIT('171978924-171978924-255115225', 1) on the logs, but errors out as:
"Timed out waiting for replication to reach 171970645-171970645-288070551,171978778-171978778-3298185533,171978924-171978924-255115225,180359242-180359242-170963125"

1.34.0-wmf.25

It may need some shaking on infra, maybe, to make it work? Otherwise I don't understand where those extra coords come from.

@Marostegui replied:

I believe this is not working as intended, not because code- I can
see now SELECT MASTER_GTID_WAIT('171978924-171978924-255115225', 1)
on the logs, but errors out as:
"Timed out waiting for replication to reach 171970645-171970645-288070551,171978778-171978778-3298185533,171978924-171978924-255115225,180359242-180359242-170963125"

1.34.0-wmf.25

It may need some shaking on infra, maybe, to make it work?
Otherwise I don't understand where those extra coords come from.

https://phabricator.wikimedia.org/T224422#5558330

Yeah, technically, all sorts of anomalies are possible, so callers should always (a) avoid DB updates based on cache data, (b) choose reasonable, possibly adaptive, TTLs for the given use case (e.g. would the world suffer if cache was wrong for X days/hours/minutes?), (c) be cognizant of CDN/ObjectCache TTL interactions (race conditions already dictate this awareness anyway, even without lost updates).

In T244340#6211682, @elukey wrote:

Side note: if not already done, I'd double check how the WarmUp route behaves when the local memcached is not available for any reason (roll restart, temporary issues for weird bugs, etc..). Ideally the timeout should be very tight, few ms, and mcrouter should behave as the GET ended up with a miss. The thing that I am worried about is that mcrouter uses the 1s timeout that we set for the main shards, but I didn't investigate a lot in the doc/code so feel free to discard if already discussed :)

Given the libketama-style consistent hashing in twemproxy and that, AFAIK, CentralAuth sessions can regenerate (notwithstanding one-off CSRF token failures and such) from the existence of the long-term centralauth_Token cookie. That would at least prevent logouts.

In my KeyValueStore refactor patches, a consume() method was added, since implementing it with TTL changes is not generically the most convenient way to best-effort atomically consume a key.

We don't really need purges to go to the gutter cache, given the low TTL there.

This entry point will just be treated by CDN like POST is w.r.t multi-DC for the foreseeable future.

In T134842#3988091, @tstarling wrote:

So this only affects users with user_token=''? The field is not nullable. Apparently only users created between March and June 2012 are affected by this:

mysql:wikiadmin@db1080 [enwiki]> select floor(user_id/1000000),count(*) from user where user_token='';
+------------------------+----------+
| floor(user_id/1000000) | count(*) |
+------------------------+----------+
|                     16 |   475413 |
+------------------------+----------+
1 row in set (13.51 sec)

mysql:wikiadmin@db1080 [enwiki]> select min(user_registration),max(user_registration),min(user_id),max(user_id) from user where user_id between 16000000 and 17000000 and user_token='';
+------------------------+------------------------+--------------+--------------+
| min(user_registration) | max(user_registration) | min(user_id) | max(user_id) |
+------------------------+------------------------+--------------+--------------+
| 20120322143714         | 20120615121417         |     16522112 |     16999995 |
+------------------------+------------------------+--------------+--------------+
1 row in set (0.54 sec)

BannerMessageGroup::registerGroupHook is triggering master queries (a) on HTTP GET requests and also (b) inside of a getWithSetCallback() call, which should use DB_REPLICA data.

In T250205#6158994, @Krinkle wrote:

In T250205#6154883, @aaron wrote:

I'm not fond of the idea of not sending purges for indirect edits

Agreed. The proposal to stop sending these purges does not stand by itself but rather would be an implementation step of "moving" the purges from one place to another (remove here, add there).

In T250205#6154883, @aaron wrote:

I'm not fond of […] using RefreshLinksJob instead of HtmlCacheUpdateJob (too slow IMO).

[…] Skipping backlink CDN purges for templates/entitites/files with millions of backlinks might work, assuming the editor is logged in and not showing things to logged out users...I can't think of a clever way to maintain expectations.

I think for someting like making a change to Template:Information or Template:Infobox, it's more valuable for logged-out users that pages get served quickly, than for those indirect changes to be applied instantly (e.g. with a cache miss resulting in a 2-60 second blank screen, awaiting an expensive reparse, possibly hitting the lower timeout threshold from GET compared to on-edit/job).

As I understand it, that last part is exactly what we're proposing. Although it would work as you'd like for unregistered editors as well, I think? They too get a full sessiont that bypasses the CDN and lasts for days/weeks (tied to their browser, incl typical session restore/continuation).

In a nut shell:

• Still purge from edit.
• Still bump page_touched recursively from a (quick) job. This means any natural cache miss or passthrough (editor with session) will still lazy re-parse as needed.
• Move recursive purges away from the quick job that does the page_touched bumps, and move it to the job that does the re-parses.

It is still useful to have a gauge of connectivity, in some cases, before attempting to use DB handles. That makes me lean towards keeping both out of convenience.

In T254608#6197273, @Krinkle wrote:

@aaron Do I understand correctly that the approach of storing the size of data, in the data stored in Memcached itself, mainly to avoid re-serialisation because we currently use the PECL implementation which deserialises natively first, and that this would be redundant after T234455?

Some more info:

aaron@mwmaint1002:~$ mwscript eval.php --wiki=testwiki
> echo strlen(json_encode(['paths'=>'ref:af18298daea159b6ca5283c0c1aa45e7155e4412', 'asOf' => time()]));
74

I suspect that the keys that cause trouble are big text/JSON blobs and ParserOutput objects, all of which don't directly get purged. READ_VERIFIED is used by MultiWriteBagOStuff already when deciding whether it is safe to do cache-aside/backfill into lower cache tiers. This could probably be integrated into mcrouter by having such calls use a route prefix that uses a warmup route. A similar flag could be added/used by WANObjectCache for other blob keys that don't receive delete()/touchCheckKey() calls. This would would at a lot of I/O resistance without making hard-to-reason-about changes to cache invalidation.

Unfortunately, I'm not seeing any xenon .log files going back that far. In terms of backend contribution, I don't see anything to do here.

From the perspective of popular/major articles, likely to have infoboxes, the extra 42.1 KB for loading the "app" JS doesn't seem crazy. I've looked through code several times and it seems reasonable. Testing with fast/slow 3G doesn't reveal obnoxious reflows or delay either. Having the edit link go directly to a Q<X> page when the JS hasn't fully loaded felt somewhat jarring, though I don't image that happening often. I don't see much editing at all given how discrete the icon is (a good thing).

I've been looking at this from time to time, and haven't found anything real problems yet. Some of the things I'm looking out for are:

Per-function flame graphs are T253679

Added two panels to the regeneration row.

I'm not fond of the idea of not sending purges for indirect edits nor using RefreshLinksJob instead of HtmlCacheUpdateJob (too slow IMO).

In T246456#6116523, @darthmon_wmde wrote:

hey @aaron, @Gilles,

could you, please, give us an update on this task? could you also tell us something we could tackle proactively that may help?

We are currently implementing the last stories and that information would help us enormously to shape our next steps.

Thanks a lot in advance!

In T133821#6092867, @Joe wrote:

At a later time, we could think of changing the logic, and make purges avoid race conditions, removing the need for the rebound purges.
One way to implement this would be the following:

• No more changes are needed at the application layer
• All purged servers join a single consumer group per datacenter. This will ensure each purge message is consumed by only one purged instance.
• This instance will take care of sending the purges to all the cache backends in the DC first, and to all the frontends afterwards

This would ensure there are no fe/be race conditions.

I agree that jobs are not always "events" or "event handler hooks" (what about jobs the subdivide or are added by maintenance scripts and so on?). There is probably a lot of stuff that can indeed be moved in that direction though.

In T250248#6060312, @Catrope wrote:

Tim mentioned this in conversation earlier but I think it's worth writing down: we'd have to ensure that the post-save page view isn't stale. It would be pretty confusing if editors didn't see their own edits.

I think the wikibase improvement simply avoided a bunch of repetitive duplicate queries, which technically would lower the hit rate.

In T157651#6039302, @Tgr wrote:

I would suggest the opposite: keep sql.php, drop patchSql.php. I don't think many people are familiar with the latter (compare patchSql vs sql docs for example) and I don't think it's terribly useful - passing a file path is more user-friendly than passing a patch name. And it does not even replace the schema vars, meaning it's actively harmful to anyone who uses table prefixes or non-default table settings.

So, IMO

• keep mysql.php which is indeed widely used at least in Wikimedia production for debugging, more user-friendly than sql.php (which channels query output through PHP which does weird things to it) and not problematic (it already requires a write flag for performing any changes - although that relies on a master/slave distinction so that could be improved, cf T249683#6039238 - and does not accidentally run updares).
• kill patchSql.php which is IMO pretty useless. (Probably worth a wikitech question to ensure it is indeed not used.)
• keep sql.php manual debugging mode, which is the only way to debug a non-MySQL server, but require an explicit --debug flag used. Do not invoke the updater (not even for variable transformations) when that's used, it seems pointless and just extra code path exposure (cf the fatal error it gave during the incident).
• keep sql.php for running scripts but require a --write flag like mysql.php does for scripts that change data. (I would even separate an admin mode for schema changes and a write mode for data changes via a restricted user.)
• if sql.php is invoked with no script file and --debug flag just exit with an error (and without creating a DatabaseUpdater or doing anything else nontrivial).

In T244058#6041722, @daniel wrote:

Another though from the TechCom meeting: we could just have the CDN cache output for old revisions and diffs for a short time (5 minutes?)

In T248962#6038821, @elukey wrote:

@aaron one thing that it would be useful is, in my opinion, having instrumentation in MediaWiki about key size volume/bytes. Even per "key family" would be enough, just to spot regressions in bandwidth usage from grafana rather than tracking them down via tcpdump. Do you think that it would be possible?

Hmm, it would help if cacheGetTree() /cacheSetTree() were replaced by getWithSetCallback() perhaps. Lots of optimizations are not used atm due to that fact.

I wonder if it would be useful for the template name to appear in the key when possible. Right now it's just an opaque hash. I doubt that many invocations of different templates have the same top-level text, so I don't see it adding much fragmentation. Maybe some statsd logging could be done instead though.

It stores the serialized naive "top frame" (e.g. headings, paragraphs, template invocation parameters) of the wikitext of pages, as well as the "sub-frames" from template invocations, upon recursive expansion. This all happens on page parse. Note that these keys do not need purges. If template X is invoked the same way on multiple pages, then parses of those pages will reuse a common sub-frame cache key for those template invocations and likewise for templates invoked from that template. So, I suppose a popular template invoked with a low enough cardinality of paramater/context bundles would trigger traffic spikes upon invalidation. The traffic would come from either (a) refreshLinks jobs or (b) page views to backlink pages that got purged via htmlCacheUpdate jobs.

The timing of this SAL entry suggests some relation:

In T244095#5983247, @Daimona wrote:

In T244095#5983011, @aaron wrote:

Is anyone working on this atm?

I don't think so, mainly because it's not clear how to proceed. Although probably, given how much assertArraySubset is used, we should probably just implement our own simplified version of it.