Page MenuHomePhabricator

CirrusSearch WAN caching should use getWithSet() instead of manual get()/set()
Open, LowPublic

Description

From IRC in #wikimedia-operations:

11:55:23 <Krinkle> If there is code storing data directly in memc bypassing getWithSet(), then that would be a problem.
11:55:26 <Krinkle> I don't know if that's the case.
11:55:49 <Krinkle> It being called out here suggests that maybe it is doing something like that, as otherwise why is it called out at all?
11:56:24 <legoktm> I think the problem is that the CirrusSearch cache is too cold to use after the switchover
11:57:32 <Krinkle> well, if the procedure laid out here is what was done in the past, then I suppose we can do it again, I mean, nothing has changed in terms of wan cache
11:57:39 <legoktm> https://gerrit.wikimedia.org/g/mediawiki/extensions/CirrusSearch/+/3df4a9b30707a2ef9ba1ebfcc84f09b915c78e15/includes/Searcher.php#642
11:57:53 <Krinkle> if its use of wan cache is new, then we can re-evaluate it indeed
11:58:37 <legoktm> it's probably not new, I'm just checking to make sure the docs are still up to date, and it seems like they are
11:59:00 <Krinkle> aye, yeah, but this does seem a bit of an anti-pattern.
11:59:30 <Krinkle> it's bypassing virtually all scale and performance levers and automation in the wanobjectcache class by not using getWithSet, I think.
12:05:16 <legoktm> Krinkle: is there a link somewhere that explains why getWithSet is better than get/set?
12:05:20 -*- legoktm is filing a bug
12:08:11 <Krinkle> legoktm: the docs for get() and set() say to consider using getWithSet, and the raw get()/set() enumerate a lot of things to consider if you call them directly. https://doc.wikimedia.org/mediawiki-core/master/php/classWANObjectCache.html
12:08:23 <Krinkle> but more generally, if you ask me, these methods just shouldn' be public in the first place.
12:09:00 <Krinkle> They probably are only public to allow for an optimisation in one or two places somewhere where we haven't bothered to accept or accomodate it in a way that is less damanging to the public API
12:09:17 <Krinkle> and they probably are only called here because someone migrated the code from wgMemc to wanCache
12:09:24 <Krinkle> which is a step in the right direction I guess.