[01:52:24] TimStarling: RoanKattouw: how would you feel about making use of hooks within core? There's a proposal to deferr JS validation of user scripts and I'd like to make the Update and Validator class namespaced under RL, and I was just thinking it might be neater if RL hooked into JavaScriptContent for relevant pages instead of that class directly referencing and "depending on" a RL service class. [01:52:47] there's probably more things in core that could be more contained if we wanted to, but Im curious what you think about the general idea. [01:54:11] I like the general idea, and I wonder if it could fit into a broader strategy of making MW more componentized and making the components more extension-like [01:55:49] I think I talked with Petr about this a few years ago [01:56:12] For example, Eric suggested making the proposed CodexModule code and other Codex integration stuff in MW an extension (including having the extension provide the @wikimedia/codex module), but we can't do that currently because then we'd shut the door on core special pages ever being able to use Codex [01:56:21] I think hooks are complex and slow for reasons that are mostly specific to the core-extension interface, like backwards compatibility and extension registration [01:56:55] I made the point that hooks do not have a monopoly on the event/listener concept [01:57:23] you can have listener arrays in core in a few lines of code -- the reason to use hooks is to take advantage of the complexity of HookContainer [02:00:30] in node.js there is a generic EventTarget/EventListener concept which applications can derive from, we don't have that in PHP [02:01:09] we could have our own, or use some library [02:01:42] I just don't want to get ourselves into a state where hooks are the only way to do callbacks [02:12:24] TimStarling: right, we have something a little bit like this today through RL\Context::getContentOverriddes which is allowing other parts of core during edit previews to inject new content into WikiModule. Likewise, it also lets the Parser remain (somewhat) agnostic of template previews. [02:13:07] We also have EventRelayer, but that's imho not suitable in terms of how we think about that today for synchronous behaviour overrides. [02:14:52] I guess the part where it curently gets tricky is where to "setup" the introduction between the two components. The "usual suspects" for this tend to be 1) Setup.php unconditional all the time, or 2) RequestContext and cross fingers on it never being ambigious or confusing. [02:16:43] actually getContentOverriddes is powered by OutputPage which is a distinction without a difference in terms of RequestContext [02:21:35] short of keeping event listeners constraint to intra-core components, it's not obvious to me what overhead HookContainer "has" to add that event listeners wouldn't. But I guess the idea is indeed to perhaps make this exclusive to core. In my imagination, a "basic" event listener class that stashes a callable and calls it if called upon for a matching event string, would mainly incur overhead in the form of setting up listeners [02:21:35] strategically/early enough so that it doesn't get missed. The ultimate version of that seems like a declaration of sorts, not unlike what extension.json does today with key-value pairs and taking them all in up-front unconditionally. I guess most of it can be deferred and done lazily until the first matching event is fired, which seems like something HookContainer would already do, or could do in the future as optimisation. [02:21:50] I guess that woudl more or less look like what Hooks looked like before 2018 and HookRunner. [02:23:14] I'm having trouble separating it. but maybe this would be a good candidate for implementing the "filter" part of T212482 [02:23:15] T212482: RFC: Evolve hook system to support "filters" and "actions" only - https://phabricator.wikimedia.org/T212482 [02:28:52] that might feel like a step back if it didn't come with an interface to enforce the callback signature somehow. but maybe php's native type hints have improved a lot that if we assume most signaatures can be expressed natively, and that copying it once can be CR'ed for correctness and thereafter live on its own in each callback. Not sure.. I imagine there's some benefit we'd lose although Im forgetting right now what that would be. [02:30:45] it'd be simpler in terms of: No support for abortable, no support for by-ref params (use an accumulating object instead). [02:31:25] possibly WordPress-style where there is a single value returned as the "output". [02:31:47] e.g. ( $thing, $a, $b ) => $thing; [02:32:34] yeah, or something simple just for intra-core.. [09:19:32] Krinkle: re read-only logic, we've been doing that since 2019 - only checking replica lag and wgReadOnlyMode in getReadOnlyReason() and eschewing isPrimaryRunningReadOnly() [16:14:12] xSavitar: could use CR on this EtcdConfig patch to help serviceops with MW-on-K8s. It's used in wmf-config early on. Let me know if you have any questions! https://gerrit.wikimedia.org/r/c/mediawiki/core/+/959801/ [16:53:15] Amir1: I'm investigating this rdbms error as side-effect of autoReconfigure: T346365 [16:53:16] T346365: PHP Notice: Undefined index: DEFAULT - https://phabricator.wikimedia.org/T346365 [16:53:32] My theory is that depooling caused it. The time correlates perfectly. https://sal.toolforge.org/production?p=0&q=dbctl&d=2023-09-09 [16:53:38] depooling db1166* [16:53:41] https://phabricator.wikimedia.org/P52345 [16:53:52] can you confirm whether that diff is a lie? [16:53:58] I suspect that dbctl actually unsets the key [16:54:02] not {} like it says there [16:54:23] so https://noc.wikimedia.org/dbconfig/eqiad.json under groupLoadsBySection would have no 'DEFAULT' set [16:55:06] This is fine imho, groupLoadsBySection shuld not require every key, only 'sectionLoads' should require every key. I also found the bug in Rdbms that makes it an error and the discrependency that makes it only an error in reconfigure and not normally [16:55:12] but wante to confirm he hypo first [16:55:21] sigh, this is weird [16:55:25] thanks for looking into it [17:01:27] Amir1: easiest might be to reproduce it. I believe the error is harmless in so far that the notice corrects to null and tunrs into an array, so no acrtual impact other than the intended effect of depoling the replicas in question. [17:01:45] could we depool them briefly for real? or would that be bad to do for no reason? [17:02:15] them*it, it's 1 hostname [17:08:55] we need to have a main script running and I can depool a replica, make sure you run it on mwmaint2xxx [17:29:15] no need for a maint script, I only want to know what the etcd response looks like [17:29:48] bu tyeah, we can also lbf->reconfigure() to confirm it fails [17:29:54] from eval.php or something [20:35:01] Krinkle, okay, will have a look tomorrow.