[04:40:38] it is kind of late here so I cannot immediately tell, but what is going on here? https://gist.githubusercontent.com/subbuss/dbe002defa6806884afc3da8edd5b9e3/raw/ec67f3e346840c71e2d0494eb98902dac7b2dba9/gistfile1.txt ... why is the preg_replace behaving weirdly there? [04:41:45] will poke at it tomorrow ... should go to bed. :) [05:12:48] subbu: I guess $1 is becoming $11972 PHP can't tell it apart [05:13:55] I determined this from the first example where you're getting "less" than $ci['value'] which, given it is literally in the replacement, is very strange. THe first character is eaten [05:14:00] which led me to questioning $1 [05:14:10] I don't know why it behaves that way, but it's a start maybe [05:14:15] o/ [11:24:43] what's the magic incantation to make jenkins run tests against sqlite? [11:25:07] check sqlite? recheck sql? [11:25:23] I just keep trying till one works [11:25:51] `check gate` should do that and other dbms [13:20:23] Krinkle, oooooh ... I see. But, that looks like a PHP bug / ambiguity. In any case, I suppose I can work around that. [13:23:52] now that I think about it, not a bug, just expected preg_* behaviour since that arg is evalualated before the preg_replace executes. [13:24:46] subbu: It's documented at https://www.php.net/manual/en/function.preg-replace.php, BTW, although oddly using the "\1" notation rather than the preferred "$1" notation. [13:26:11] anomie, got it. thanks. i can use that documented workaround for disambiguating this. [15:13:42] anomie: Hm.. I'm not seeing in the docs why $11 would be 11th match and \11 be 1st match with literal 1. I do see it mention you can escape it like \{1]1 or ${1}1 [15:14:51] subbu: I'm not sure it's a bug because because the strings are concatenated so PHP doesn't see anything other than "$11972". At east it's stopping after numbers higher than 99. But if this were a callback where you substitute $m[1]."1972" then it would not happen. [15:15:00] Just a downside of using the magic string method [15:15:21] It looks like this takes arbitrary input, so might be another reason to avoid it entirely as there [15:15:26] yes, as i said earlier, it is not a bug. [15:15:29] s no way we can guruantee it wholistically afaik [15:15:40] other than using a callback [15:15:49] butmaybe there's a way to escape it generally? I dunno [15:16:07] ah ok, missed that [15:17:41] Krinkle: It doesn't say that. It says "\11" is ambiguous, without directly mentioning "$11" and only implying how the ambiguity is resolved in saying "you cannot use the familiar \1 notation". Then it says you can resolve the ambiguity with "${1}1", again implying "$11" is the "normal" interpretation. Also it doesn't say that "\{1}1" is even an option. [15:19:11] anomie: right, but do you mean that with $11 it prefers match 11 and with \11 it is ambiguous and maybe sometimes will pick 1 of 11 doesn't exist? [15:19:15] That's interesting. [15:19:54] Krinkle: I mean it'll do match 11 for both. At least that's how I read the documentation there. [15:21:05] anomie: OK, I understand your original comment now, I misread it. [15:21:36] You were describing the fact that \11 is documented as ambiguous, pointing out that $11, while seemingly having the same issue, is not mentioned. I misinterpreted it as you saying we should use \11 instead of $11 to solve the issue [15:38:09] curious why JS isn't affected by this same bug ... since that code is also src.replace(some-re-here, "$1" + val) and val could be "1" for ex. [15:41:11] subbu: where PHP reserves 1-99, JS reserves 1-9 only [15:41:12] http://es5.github.io/#x15.5.4.11 [15:41:15] thus naturally avoiding the bug [15:41:20] if you need more, gotta use a callback [15:41:35] nope, I'm wrong [15:41:42] the doc is confusingly split up between $n and $nn [15:41:44] I don't know then [15:42:01] "If nn>m, the result is implementation-defined." [15:42:41] aha! [15:42:46] I guess maybe V8/Node.js does some smartipantness to only do the replacement if the match exist so $11 would be $1 + '1' if there is no 11nth match. [15:43:02] that was my original guess. [16:16:31] and presumably Microsoft disagreed some decades ago and hence it remains implementation-defined. [16:16:52] or maybe one of those things carefully copied from the original Netscape implementation. [16:17:13] might be worth revisiting post-EdgeHTML/Chakra [17:41:12] duesen_, Reedy: "check sqlite", but it looks like it doesn't work if you also leave inline comments. Also "check php", "check php5", "check zend", and "check postgres" do the same thing. https://gerrit.wikimedia.org/r/plugins/gitiles/integration/config/+/master/zuul/layout.yaml#574 [17:57:49] duesen_: The comment incantations are documented as of a few weeks ago at https://integration.wikimedia.org/zuul/ [17:57:54] I've added them to the pipeline descriptions [17:59:19] It could still use some improvement to specify that the "php" pipeline is where sqlite/pgsql run. This is covered by "PHPUnit jobs that run in 'gate' but optimised out of 'test'." but not obvious. [19:15:40] duesen_: Reading through the stable interface draft. In terms of what it communicates, I like the changes since last time. I do think we should maybe try to lower the number of annotations a bit. E.g. combining more "behaves the same" together leaving "intent" more for social/documentation. Or maybe as parameter to the annotation if it's useful to document in-place. Right now it feels like a lot of stuff at once, feels like a lot of work [19:15:41] as developer even if it might not be that way in the end. [19:16:11] I also found confusing that after the intro that lays out that we're taking about core, the first section talks about non-policy of what extensions are recommended to do, which seems out of place. [19:21:40] Krinkle: thanks for the feedback. You you copy to the ticket, so it doesn't get lost? I'm a bit distracted right now. [19:22:05] k [19:22:19] I gree that the number of annotations is high, but I didn't see a good way to condense them. Let's talk on the ticket. [20:16:58] anomie: I think I remember that you had a fancy way of making eval.php write log messages to the terminal. Am I remembering that right? I know I knew how to do this stuff N years ago, but it has fallen out of my brain. [20:17:35] bd808: -d 1 or -d 2? [20:18:31] oh, shiny! I think this used to be harder :) [20:19:45] Probably the junk I used before I found out about that option. [22:26:02] duesen_: btw https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/528924/ is back to you. Any blockers for now? [22:27:04] There's a number of simplifications and deprecations we'd like to do. the motivation wasn't actually decoupling, although that's cerainly nice as well :) [22:28:34] Krinkle: I'll look over it tomorrow. but I'm afraid I don't understand the details well enough to give a +2. [22:29:00] last time i tried to understand how cache invalidation actually works, i failed miserably. [22:29:20] duesen_: OK. I can do that part. Did already actually. [22:29:26] (twice) [22:29:49] ok, I'll look over the DI aspect then [22:30:18] I know you did that to some extend already, but want to make sure we've captured the ones you think should be blocking. [22:30:36] (obv. disagree where you like.) [22:31:08] Krinkle: by the way... regarding post-send deferred updates... if they are not covered by chrono-protection, how are they conceptually different from jobs? [22:35:00] duesen_: They are not. [22:35:18] Other than a way of jumping the queue for stuff you want to happen soon. [22:35:39] and for stuff that might be complex or inefficient to persist and revive in the queue [22:36:05] I don't rule it out long-term to not have post-send updates. But I'm not pushing for it either. [22:37:14] Krinkle: ok, thanks for the clarification. I only last week understood that these are not covered by chrono-protect. [22:37:33] it actually seems like that might be a nice thing to have - it would make use of the round-trip time in a safe way. [22:38:42] we do that already though. we write to the master pre-send (we have to, so that we can tell if you it failed). Then update the last-seen master pos in CP. Then while you roundtrip, your next GET req will read from a replica and might not have to wait long or at all. [22:39:45] But if stuff uses post-send and you want to see it serially, sounds to me like it shouldn't be using post-send. Kinda comes back to what I was mentioning on that integration test task is that I understand it theoretically but I don't see a MW use case for it per se. [22:41:06] You could do all writes post-send if chrono-protect covered it... though I guess that's bad in case of an error. You#d have to funnel errors through the chrono-protect mechanism as well. [22:41:15] anyway, that's just idle musing [22:41:21] If we allow waiting for post-send that would imply all users have their request serially processed, which is not what CP is meant to do. We only (currently) want new requests to see what the last finished request saw. Given a previous request might still be doing post-send, we can't wait for that unless we disallow concurrency at all. [22:42:11] yea, you'd have to use jobs for that [22:42:14] and yeah, with post-send being like jobs, it's not given that it'll be reasonably quick [22:42:24] so we actualy don't want to delay user reqs on it [22:42:54] I'm still convinced that we need this kind of "observability" for testing. maybe some bot activities would benefit from it as well, but I can't point to a real life use case [22:43:19] duesen_: bots and tests are in charge of their requests so they can make them serially in the first place. [22:43:33] And disabling post-send can be configured if need be. that seems fair [22:45:20] duesen_: as of this commit, there's a nice place for such config flag to go: https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/538362/7/includes/MediaWiki.php [22:46:56] That might even make it possible to not need to use RunJobs, E.g. if you set JobRunRate=INF and AsyncJobs=false, maybe that will do what you want (together with postSendStrategy=3) [22:48:56] Krinkle: bots and tests *cant* make their requests serially in a way that makes post-send updates observable in a reliable way. that's the problem. [22:49:27] why do you want it to be a config flag, rather than per request? [22:49:35] is there a problem with doing it per request? [22:50:29] duesen_: less abuse potential, less implemenattion/maintenance complexity [22:50:53] I don't want to have to support that in any way for "real". [22:52:17] and i don't want to write tests for "unrelad" ;) [22:52:26] "unreal" even [22:52:58] But we can live with the restriction, I think [22:53:03] right, but that's what it ends up being either way. not doing post-send at post-send means it will behave in a non-prod like way. [22:53:25] I just don't like the fact that this means that our suite of integration tests cannot pass wihtout a config change [22:53:26] I would still like to hear an example of something post-send you want to see in subsequent requests. [22:54:11] if we don't want to see it in subsequent requests, why are we writing it to the database in the first place?... [22:54:39] RecentChanges is the primary example (as far as I can tell, it is post-send). [22:54:52] last time we checked it on the the task, it wasn't [22:54:55] I started to do a survey the other day, but didn't finish it [22:54:58] the same mistake was in the wdio tests, which I also removed [22:55:10] i could still be mistaken of course [22:55:36] RecentChange::notifyEdit is using POSTSEND [22:55:49] db-writes also generally shouldn't be post-send, precisely because they are not CP, hence no benefit to doing it from a job. [22:56:51] ManualLogEntry::publish also does POSTSEND [22:57:10] WikiPage::insertRedirect is POSTSEND [22:57:39] User::incEditCount [22:57:46] I see. So we do revision PRESEND, but things on other pages are postsend. [22:57:56] logically "further away" [22:58:19] looks like it [22:58:22] and for real users they will appear later, and things like RC also has replag warnings where we tell users it might be out of date. [22:58:27] but we still want integration tests for them [22:59:16] Right, but that means observing things that are not guaranteed by the software. that's fine, but is as such why it is "special" and different. opting in per-req seems fine, but I'd still want the ability itself behind a feature flag. [22:59:22] looking at the uses of POSTSEND, all of them seem to be doing database inserts, by the way [22:59:28] (what else would they be doing?) [22:59:30] Things like not having a replica DB and using jobs sync will also simplify this [23:00:43] Yes. [23:01:05] Actually, having a per request header, and a setting to *ignore* that header would be ideal [23:01:37] unless it's unsafe top have that on by default [23:06:26] Krinkle: anyway, time for silly youtube videos and bed ;)