Page MenuHomePhabricator

Mapdata API should use the FlaggedRevs stable revision ParserCache when appropriate
Closed, ResolvedPublic8 Estimated Story Points

Description

FlaggedRevs intercepts stable revision pageviews and stores ParserOutput in the ParserCache under a different key than the latest revisions. APIs on the other hand will use RevisionOutputCache, which will cause duplicated parsing work and lower cache hit rates. We want to fix this for at least the mapdata API, since we know it will be called for every page with a map.

Quick fix

  • Add code to the mapdata API to switch to the FlaggedRevs cache for stable pages.

Proper fix

  • Add a new hook to ParserOutputAccess to allow extensions to provide a ParserOutput when appropriate.
  • Implement this hook in FlaggedRevs, to provide stable-pcache content if it exists and store there after parsing.

Details

ProjectBranchLines +/-Subject
mediawiki/extensions/Kartographerwmf/1.39.0-wmf.10+6 -2
mediawiki/extensions/Kartographermaster+6 -2
mediawiki/extensions/Kartographermaster+0 -8
operations/mediawiki-configmaster+1 -1
mediawiki/extensions/Kartographermaster+1 -1
mediawiki/coremaster+39 -56
mediawiki/coremaster+82 -5
mediawiki/extensions/FlaggedRevsmaster+168 -0
operations/mediawiki-configmaster+1 -0
mediawiki/extensions/Kartographermaster+118 -4
mediawiki/coremaster+83 -120
mediawiki/coremaster+33 -210
mediawiki/coremaster+21 -38
mediawiki/coremaster+11 -14
integration/configmaster+3 -3
mediawiki/coremaster+64 -1
mediawiki/coremaster+8 -16
mediawiki/extensions/FlaggedRevsmaster+118 -16
Show related patches Customize query in gerrit

Event Timeline

Change 773446 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/FlaggedRevs@master] [WIP] Implement multi-cache hooks

https://gerrit.wikimedia.org/r/773446

Change 774419 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/FlaggedRevs@master] Extract cache handling into a new class

https://gerrit.wikimedia.org/r/774419

Change 773361 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/core@master] New hook to divert ParserOutputAccess

https://gerrit.wikimedia.org/r/773361

thiemowmde set the point value for this task to 8.Mar 30 2022, 8:42 AM
thiemowmde set Final Story Points to 13.

Change 774419 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Extract cache handling into a new class

https://gerrit.wikimedia.org/r/774419

Change 776901 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Don't cache old revisions in ParserOutputAccess' local cache

https://gerrit.wikimedia.org/r/776901

Change 776900 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Make old vs. latest revision more obvious in ParserOutputAccess

https://gerrit.wikimedia.org/r/776900

Change 776937 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Untangle dependencies between PoolWorkArticleView subclasses

https://gerrit.wikimedia.org/r/776937

Change 776900 merged by jenkins-bot:

[mediawiki/core@master] Make old vs. latest revision more obvious in ParserOutputAccess

https://gerrit.wikimedia.org/r/776900

Change 777360 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/core@master] [WIP] Experimenting with pooled parse workers

https://gerrit.wikimedia.org/r/777360

Change 777376 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Rearrange PoolWorkArticleViewCurrent::fallback() for clarity

https://gerrit.wikimedia.org/r/777376

Change 777382 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Move "dirty" logic to PoolWorkArticleView subclass that uses it

https://gerrit.wikimedia.org/r/777382

Change 777408 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Change ParserOutputAccess workers to work with Status objects

https://gerrit.wikimedia.org/r/777408

Change 777750 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/core@master] Test for PoolWorkArticleViewCurrent::fallback

https://gerrit.wikimedia.org/r/777750

Change 777750 merged by jenkins-bot:

[mediawiki/core@master] Test for PoolWorkArticleViewCurrent::fallback

https://gerrit.wikimedia.org/r/777750

Change 778279 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Kartographer@master] Workaround for FlaggedRevs stable-revision caching

https://gerrit.wikimedia.org/r/778279

Change 778287 had a related patch set uploaded (by Awight; author: Awight):

[integration/config@master] Kartographer tests depend on FlaggedRevs

https://gerrit.wikimedia.org/r/778287

awight removed awight as the assignee of this task.Apr 7 2022, 1:15 PM
awight updated Other Assignee, added: thiemowmde.
awight updated Other Assignee, removed: thiemowmde.

Change 778287 merged by jenkins-bot:

[integration/config@master] Kartographer tests depend on FlaggedRevs

https://gerrit.wikimedia.org/r/778287

Change 777376 merged by jenkins-bot:

[mediawiki/core@master] Rearrange PoolWorkArticleViewCurrent::fallback() for clarity

https://gerrit.wikimedia.org/r/777376

Change 777382 merged by jenkins-bot:

[mediawiki/core@master] Move "dirty" logic to PoolWorkArticleView subclass that uses it

https://gerrit.wikimedia.org/r/777382

Change 777360 abandoned by Awight:

[mediawiki/core@master] [WIP] Experimenting with pooled parse workers

Reason:

https://gerrit.wikimedia.org/r/777360

Change 777408 merged by jenkins-bot:

[mediawiki/core@master] Change ParserOutputAccess workers to work with Status objects

https://gerrit.wikimedia.org/r/777408

I was able to reproduce the CI error quite simply, by commenting out all of my local settings and including DevelopmentSettings.php

It was $wgKartographerVersionedMapdata = true;. Of course... without this setting the mapdata API won't try to fetch data from the older revision.

awight renamed this task from Integrate FlaggedRevs ParserCache into the ParserOutputAccess service when fetching the stable revision to Mapdata API should use the FlaggedRevs stable revision ParserCache when appropriate.Apr 13 2022, 12:22 PM

Change 778279 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Workaround for FlaggedRevs stable-revision caching

https://gerrit.wikimedia.org/r/778279

thiemowmde added a subscriber: thiemowmde.

Remaining patches are all optional or even WIP.

Change 778279 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Workaround for FlaggedRevs stable-revision caching

https://gerrit.wikimedia.org/r/778279

I ran several tests on the beta cluster[1] with this to see if we ever get a Stable cache miss in the logs. Currently I got nothing there. So I assume it works.

[1] https://de.wikipedia.beta.wmflabs.org/w/index.php?title=Maptests

Change 785852 had a related patch set uploaded (by Awight; author: Awight):

[operations/mediawiki-config@master] [beta] Stash all logs for the Kartographer extension

https://gerrit.wikimedia.org/r/785852

Change 785852 merged by jenkins-bot:

[operations/mediawiki-config@master] [beta] Stash all logs for the Kartographer extension

https://gerrit.wikimedia.org/r/785852

Change 786289 had a related patch set uploaded (by Awight; author: Awight):

[operations/mediawiki-config@master] Watch for mapdata cache misses in production

https://gerrit.wikimedia.org/r/786289

Change 773446 abandoned by Awight:

[mediawiki/extensions/FlaggedRevs@master] Implement multi-cache hook

Reason:

easing out of this rabbit hole...

https://gerrit.wikimedia.org/r/773446

Change 773361 abandoned by Awight:

[mediawiki/core@master] New hook to divert ParserOutputAccess

Reason:

Dropping in favor of a one-off workaround for our use case.

https://gerrit.wikimedia.org/r/773361

I ran several tests on the beta cluster[1] with this to see if we ever get a Stable cache miss in the logs. Currently I got nothing there. So I assume it works.

Once logging was enabled, we were also able to demonstrate the rare case when the stable cache is missed, and verified that the code path is executed and result looks correct.

Change 776937 merged by jenkins-bot:

[mediawiki/core@master] Untangle dependencies between PoolWorkArticleView subclasses

https://gerrit.wikimedia.org/r/776937

Change 773446 abandoned by Awight:

[mediawiki/extensions/FlaggedRevs@master] Implement multi-cache hook

Reason:

easing out of this rabbit hole...

https://gerrit.wikimedia.org/r/773446

For what it's worth, I think you made the right call to post-pone refactoring of this for now. It's definitely something we'll want to circle back to at some point, but it seems more risk to me than is useful and relevant stakeholders are busy with other refactors already that this is fine to call directly for the time being.

WMDE-Fisch moved this task from Demo to Done on the WMDE-TechWish-Sprint-2022-04-13 board.

Just a note that the code contains:

* FIXME: Once T304813 is resolved, MediaWiki core will be able to dynamically select the
* correct cache.  Until then, we're explicitly using the FlaggedRevs stable-revision cache to

Reading the above i'm guessing this part was actually not done as part of this ticket, so that comment isn't fully correct any longer ?

Just a note that the code contains:

* FIXME: Once T304813 is resolved, MediaWiki core will be able to dynamically select the
* correct cache.  Until then, we're explicitly using the FlaggedRevs stable-revision cache to

Reading the above i'm guessing this part was actually not done as part of this ticket, so that comment isn't fully correct any longer ?

Good catch. We'll create an extra ticket for that solution and can alter the comment then.

Change 788301 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Kartographer@master] Update TODO with new task number

https://gerrit.wikimedia.org/r/788301

Change 788301 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Update TODO with new task number

https://gerrit.wikimedia.org/r/788301

Change 786289 merged by jenkins-bot:

[operations/mediawiki-config@master] Watch for mapdata cache misses in production

https://gerrit.wikimedia.org/r/786289

Mentioned in SAL (#wikimedia-operations) [2022-05-10T07:48:24Z] <awight@deploy1002> Synchronized wmf-config: Config: [[gerrit:786289|Watch for mapdata cache misses in production (T304813 T300712)]] (duration: 00m 50s)

Change 791018 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Kartographer@master] Duplicate "latest revision may be special" logic from FlaggedRevs

https://gerrit.wikimedia.org/r/791018

Change 791027 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Kartographer@master] Remove stable cache workaround logging

https://gerrit.wikimedia.org/r/791027

Change 791248 had a related patch set uploaded (by Awight; author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Kartographer@wmf/1.39.0-wmf.10] Duplicate "latest revision may be special" logic from FlaggedRevs

https://gerrit.wikimedia.org/r/791248

Change 791018 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Duplicate "latest revision may be special" logic from FlaggedRevs

https://gerrit.wikimedia.org/r/791018

Change 791248 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/extensions/Kartographer@wmf/1.39.0-wmf.10] Duplicate "latest revision may be special" logic from FlaggedRevs

Reason:

wmf.12 will come this week anyway.

https://gerrit.wikimedia.org/r/791248