Page MenuHomePhabricator

Parseroutput cache issue with videojs
Open, NormalPublic

Description

< Krinkle> ParserCache::save Memc set key: enwiki:pcache:idoptions:12108 - ok: true
< Krinkle> ParserCache::save Memc set key: enwiki:pcache:idhash:12108-0!canonical!tmh-videojs true

< Krinkle> Logstash: channel:AdHocDebug OR channel:ParserCache OR message:parser OR e:parsercache
< Krinkle> ParserOutput key valid, but rejected by RejectParserCacheValue hook handler.
< Krinkle> Prod extensions using this MW hook: Wikibase, ops/mediawiki-config, Kartographer, TimedMediaHandler

Krinkle> So if you user pref and current object is different from site config, it always refuses to persist it
Krinkle> so likely just induced by the beta pref more popular

See also:
https://gerrit.wikimedia.org/g/mediawiki/extensions/TimedMediaHandler/+/a9a3b431d5c175778b076b7d6c230aa8280c121f/includes/TimedMediaHandlerHooks.php#525

So on RejectParserCacheValue of TimedMediaHandler only considers the site default, and not the activePlayerMode for the player. This was done for a site preference switch, but didn't account for user preference switches via beta player mode and the parsercachekey already being adapted according to this activePlayerMode.

The mode has been available for a while already. Likely got more pervasive as parsercache values started expiring, more and more people get enrolled in the betafeature etc. Additionally the issue is only really noticeable if the parsercache performance actually matters aka. big pages only

< James_F> https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/530187 is the ncy disable patch if it's needed.
< James_F> It'll be messy, but it'd stop people adding the beta feature.
< James_F> (And as they touch their preferences, existing people would get emptied out.)

Details

Related Gerrit Patches:
mediawiki/extensions/TimedMediaHandler : masterAdjustments to RejectParserCacheValue for TMH player mode
mediawiki/extensions/TimedMediaHandler : masteronRejectParserCacheValue: Correct name of target videojs module; add comment

Event Timeline

TheDJ created this task.Aug 14 2019, 6:05 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 14 2019, 6:05 PM
TheDJ updated the task description. (Show Details)Aug 14 2019, 6:07 PM

High or UBN priority?

TheDJ triaged this task as High priority.Aug 14 2019, 6:09 PM
TheDJ updated the task description. (Show Details)
TheDJ added subscribers: Jdforrester-WMF, Krinkle.

Change 530191 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/TimedMediaHandler@master] onRejectParserCacheValue: Correct name of target videojs module; add comment

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

brion added a subscriber: brion.Aug 14 2019, 6:31 PM

I think default mode is checked because there's a separate pcache suffix for folks using the beta-mode -- this is targeted at clearing the non-beta-mode entries after a site-config switch.

So it probably shouldn't be rejecting anything on the beta-mode suffix variant...?

I also think it's unwise to call purgeSquid here; it won't actually work in all views (eg it never gets called if you were on a Varnish cache hit). Simply rejecting the pcache should be enough for logged-in views without unnecessarily purging other cache data, whereas if we need to reset all the pages with A/V to handle the updated output & modules, then we should have a script purge just those pages.

brion added a comment.Aug 14 2019, 6:50 PM

Note the parser cache key is not available to the hook. Fun!

I think what will work is:

  • change to use activePlayerMode instead of defaultPlayerMode
  • don't actively purge anything

This will allow old default cached page views to update transparently after the default is switched (if they get past Varnish cache layer), and should be harmless for users in the beta group, who see a different suffix on their cache keys.

(If we find we need to do a mass-update of pages after the switch (to deal with cached old modules that won't exist in RL eventually) then we'll want to purge just those pages with a batch or something.)

Change 530200 had a related patch set uploaded (by Brion VIBBER; owner: Brion VIBBER):
[mediawiki/extensions/TimedMediaHandler@master] Adjustments to RejectParserCacheValue for TMH player mode

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

Change 530191 merged by jenkins-bot:
[mediawiki/extensions/TimedMediaHandler@master] onRejectParserCacheValue: Correct name of target videojs module; add comment

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

Change 530200 merged by jenkins-bot:
[mediawiki/extensions/TimedMediaHandler@master] Adjustments to RejectParserCacheValue for TMH player mode

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

jbond added a subscriber: jbond.Aug 15 2019, 9:07 AM
CDanis added a subscriber: CDanis.Aug 16 2019, 12:43 PM
Jdforrester-WMF lowered the priority of this task from High to Normal.Aug 18 2019, 5:12 PM

Provisionally we think this is fixed in master for the current usage in production (and will roll out with the train this week); there are further fixes we'll want to do before closing this task, however.