ParserOutput cache issue with videojs
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	TheDJ
	Aug 14 2019, 6:05 PM

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

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

Subject	Repo	Branch	Lines +/-
Remove redundant check against the value of beta user option	mediawiki/extensions/TimedMediaHandler	master	+1 -4
Limit parsercache hash vary to Beta and prepare for default roll-out	mediawiki/extensions/TimedMediaHandler	master	+58 -17
Consistently use MainConfig service in hooks	mediawiki/extensions/TimedMediaHandler	master	+7 -15
Fully drop ParserCache reject	mediawiki/extensions/TimedMediaHandler	master	+0 -25
Adjustments to RejectParserCacheValue for TMH player mode	mediawiki/extensions/TimedMediaHandler	master	+2 -9
onRejectParserCacheValue: Correct name of target videojs module; add comment	mediawiki/extensions/TimedMediaHandler	master	+7 -1

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Open	None	T49145 Formally deprecate jQuery UI after we've stopped using jQuery UI in extensions and core
Open	None	T100270 Replace use of jQuery UI and MW UI with OOUI across all Wikimedia-deployed extensions and core
Declined	None	T89496 Replace TimedMediaHandler's use of deprecated jQuery UI with OOUI
Open	None	T158181 Aim for workflow equivalence for MediaWiki on desktop and mobile web
Resolved	• brooke	T62565 TMH totally broken on mobile
Resolved	TheDJ	T93544 Consider alternatives for EmbedPlayer
Resolved	TheDJ	T96504 Video subtitles delay
Resolved	TheDJ	T121099 Playing a video on iOS when using desktop site does not allow pausing the video or see the control bar
Resolved	TheDJ	T217085 Enable TimedMediaHandler video player in articles on mobile (instead of hyperlink to unplayable video)
Declined	None	T203413 a small audio ogg file does not get played in full
Resolved	Jdforrester-WMF	T306971 Archive Kaltura Player Phab project and decline all its open tasks
Resolved	Jdforrester-WMF	T306972 Update on-wiki documentation after replacing Kaltura player with Video.js
Resolved	• brooke	T100106 Replace Kaltura player with Video.js
Resolved	Krinkle	T230505 ParserOutput cache issue with videojs

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

Paladox subscribed.Aug 14 2019, 6:06 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

gerritbot added a project: Patch-For-Review.Aug 14 2019, 6:24 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.

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

Maintenance_bot removed a project: Patch-For-Review.Aug 14 2019, 10:10 PM

jbond subscribed.Aug 15 2019, 9:07 AM

CDanis subscribed.Aug 16 2019, 12:43 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.

ReleaseTaggerBot edited projects, added MW-1.34-notes (1.34.0-wmf.20; 2019-08-27); removed MW-1.34-notes (1.34.0-wmf.19; 2019-08-20).Aug 20 2019, 8:01 PM

Krinkle added a parent task: T100106: Replace Kaltura player with Video.js.Jul 7 2021, 11:42 PM

Ladsgroup mentioned this in T248418: Roll out videojs as the only video/audio player on all Wikimedia wikis.Jan 27 2022, 5:29 PM

Change 757985 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/TimedMediaHandler@master] Fully drop ParserCache rejecting and fragmentation

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

gerritbot added a project: Patch-For-Review.Jan 28 2022, 10:30 PM

Can someone tell me why fragmentation of PC is not enough and you need to hook into onRejectParserCacheValue? How is it different from user language for example?

TheDJ mentioned this in T249199: Deprecate PageRenderingHash hook.Jan 31 2022, 10:19 PM

Krinkle claimed this task.Feb 1 2022, 8:08 PM

Change 758985 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/TimedMediaHandler@master] Consistently use MainConfig service in hooks

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

Krinkle renamed this task from Parseroutput cache issue with videojs to ParserOutput cache issue with videojs.Feb 1 2022, 11:50 PM

Change 758989 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/TimedMediaHandler@master] Hooks: Limit parsercache vary to Beta and prepare for default roll-out

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

Change 757985 abandoned by Ladsgroup:

[mediawiki/extensions/TimedMediaHandler@master] Fully drop ParserCache reject

Reason:

Yes. After discussions, we decided not to do it this way

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

Change 758985 merged by jenkins-bot:

[mediawiki/extensions/TimedMediaHandler@master] Consistently use MainConfig service in hooks

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

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.21; 2022-02-07).Feb 2 2022, 11:00 AM

Change 758989 merged by jenkins-bot:

[mediawiki/extensions/TimedMediaHandler@master] Limit parsercache hash vary to Beta and prepare for default roll-out

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

I boldly close this. I suggest measuring pc after each rollout (to any wiki)

LSobanski subscribed.Feb 7 2022, 11:49 AM

Change 759311 had a related patch set uploaded (by Krinkle; author: Func):

[mediawiki/extensions/TimedMediaHandler@master] Remove redundant check against the value of beta user option

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

Change 759311 merged by jenkins-bot: