[00:51:13] New Memcached diagram: https://wikitech.wikimedia.org/wiki/Memcached_for_MediaWiki#/media/File:Wikipedia_Memcached_flow_2025.png. Changes since 2022: add MicroStash, remove on-host tier 3 parser cache, include PC and Memc overall capacity, simplify and clarify various other aspects. Old version: https://wikitech.wikimedia.org/wiki/Infographics#/media/File:Wikipedia_Memcached_flow_2022.png [01:16:09] Krinkle: re T389559, that sounds good to me [01:16:10] T389559: Unable to view page information on [[Difracció]] article at ca.wikipedia.org - https://phabricator.wikimedia.org/T389559 [10:39:34] tgr|away: regarding Sinon, I took a stab at T389450 [10:39:34] T389450: Upgrade from Sinon 1.x to Sinon 18 - https://phabricator.wikimedia.org/T389450 [11:52:54] thanks for keeping the diagrams updated, they are very useful! [11:55:08] the old diagram had LocalClusterInstance and the new has the MicroStash, but those are two different things, right? [11:55:36] with one being DC-local, and the other primary-DC [20:10:32] Krinkle: can you have a look at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1130105 (I also commented on T386217) [20:10:32] T386217: 1.44.0-wmf.22 deployment blockers - https://phabricator.wikimedia.org/T386217 [21:36:52] tgr_: indeed, not a direct replacement and also various other things changed. Previously I more or less had to include the internal LocalClusterCache because that's how rate limiter and CP worked by default, they used that internal serviec directly despite intentgionally not being exposed to service wiring (things should use WANCache instead mostly), but there were numerous use casees that needed direct access, but each had their own [21:36:52] config settings that WMF all pointed to mcrouter-primary-dc and core defaulted to false/null with fallback to LocalCluster/MainCacheType. [21:37:20] We've since recognised these use cases as MicroStash (ref T336004), and gotten rid of the config complexity. [21:37:21] T336004: Recognize 4th cache service interface in MediaWiki (Migrate ConfirmEdit tokens from MainStash to mcrouter-primary-dc) - https://phabricator.wikimedia.org/T336004 [21:37:45] So to a first approximation nothing uses LocalClusterCache directly and so doesn't need to be in the diagram. [23:19:11] AaronSchulz: left a +1, not sure what you'd like me to review exactly [23:20:00] Krinkle: sounds good. You had a large history comment on the task so I figured you can see if we missed something. [23:20:43] AaronSchulz: history != current code :) [23:21:17] AaronSchulz: where do hooks consume this edit result? It sounds like FlaggedRevs is reading/expecting this to exist sometime between the previous set() point and the new domain event? [23:23:27] I can't speak to the timing of those, but I'd pick one way rather than a mixture. e.g. either set where it was set before, and so the hook from the derived page updater is allowed to see it, or move both the set() and the hook. Presumably we can't move the hook without a breaking change, so that means keeping both where they were I suppose? [23:24:27] It looks like the `this->editResultCache->set(` in includes/Storage/DerivedPageDataUpdater.php is indeed logically where it was before the recent refactor (now without the one-use private function wrapper) [23:25:41] The condition `$editResult->isRevert() && $this->useRcPatrol` does look like it's different from the `approved` field that was there before. [23:25:55] that'd be where I'd look a bit closer to the implications [23:41:33] Krinkle: PageTriage checks the cache indirectly by calling reallyMarkPatrolled(). FlaggedRevs seems to have its own markRevisionPatrolled() method that doesn't use that, but it does call approveRevertedTagForRevision() directly. [23:43:36] I see, yeah, so storing it from the same point as before seems fine then. Medium term I'd like to explore removing the feature as it seems quite an edge case that might actually have undesirable effect on UX even when it does work. [23:44:00] Not sure we still have a PM for this since it was developed via GSOC and the team has undergone two reorgs since. [23:44:50] Krinkle: it looks like FR also had the bug of always calling approveRevertedTagForRevision() on autoreview regardless of isRevert(), so this patch also mitigates that needless jobs. [23:46:14] Krinkle: the goal should definitely be to get rid of stashing edit results, even if it means scanning for reverts and checking rev_sha1 (or I guess content_sha1 if that goes) for patrol-after-the-fact. [23:46:33] I'll see if I can bring jtweed upto speed next week and get his input on this from an end-user perspective. E.g the trade-off can either go towards treating RC-wikis the same as non-RC wikis (add the tag to any edit that is effectively a revert, regardless of patrol status) or toward RC-wikis keeping the current behaviour where the tag is limited to reverts via rollback or trusted users (removing the change-after-the-fact behaviour). [23:47:15] What baffles me is that on non-RC wikis it just treats all edits as trusted, so that makes it even more surprising. I wonder if commnunities actually know that the tag is missing on their wiki until/unless the undo is patrolled. [23:48:10] Yeah, we can queue a job on action=patrol and do the scan there. It's not exactly high traffic or that expensive. [23:48:21] then we could punt the product question, but let's try. [23:48:24] that code flow is extremely baffling too...to the level that it should have blocked the original feature until things could be cleaned up. [23:48:46] yeah, it was a bit rushed as part of GSOC. I don't think it was officially owned. [23:49:24] Although I suppose defacto Growth would have been aware of/blessed the tagging feature in principle, they might not have reviewed or taken ownership of the exact details. [23:50:56] I do note that Roan (Growth team at the time, maintainer of RC) reviewed it and left a comment similar to mine. at https://phabricator.wikimedia.org/T259103#6347306 [23:51:07] but it seems it went ahead as-is regardless [23:52:18] > In case the main stash method fails, it falls back to trying to find the EditResult in the ct_params field of revert change tags. [23:52:19] https://gerrit.wikimedia.org/r/c/mediawiki/core/+/618784 [23:52:27] I'd have questioned that part as well. [23:52:41] We're storing editresult permanently in the changetags table? interesting. [23:54:00] I didn't know that column existed. [23:54:56] * Krinkle closes tab before another day disappears [23:55:03] anyway, I hope some of that helps :)