[03:37:02] dpifke: saw a couple email with seas of arclamp warnings that I don't think we've seen before. I it may've been a one-off while testing or deploying. is that stil the case, or perhaps something new? or improved telemetry? [15:49:30] Those errors are from corrupt log lines. Should no longer be written as of https://gerrit.wikimedia.org/r/c/performance/arc-lamp/+/641474/, and I cleaned up the old data using a shell one-liner. [15:49:55] I should have caught that when this was in beta, but the cron messages got lost in a flood of other noise from deployment-prep. :( [15:51:03] I have some ideas on how to make the message more useful (e.g. by outputting filenames) but definitely won't get to it this week. [16:42:51] Hmm so something recently did introduce the issue? Not sure I understand why Python3 would make longer lines. Or is length the side effect of bad encoding? [16:42:59] Anyway, seems fixed now [16:43:05] Nice catch [16:59:58] AaronSchulz: train blocked, commons issue again. asking releng to coordinate with you, I'm blocked out on vue.js taskforce. [17:00:24] maybe it just needs to be ported to both branches, feel free to ping for a +2 but otherwise unavail :/ [17:08:23] In Python 2, byte arrays and strings are basically interchangeable, because the interpreter doesn't care about string encoding. In Python 3, you have to explicitly convert between the two, because knowing the encoding is a mandatory part of the conversion. [17:08:59] E.g. you have to use `bytes.encode('utf-8')` instead of just casting `str(bytes)`. [17:09:21] As a result of this, functions which expect a string fail in interesting ways when passed byte arrays. [17:09:37] (s/decode/encode/) [17:10:37] `print` is one of those, which is what was corrupting the ArcLamp logs. Lines were being written as `b'foo'`. [17:11:41] Fix was to use `write(bytes)` instead of `print(str)`. [17:13:18] (We could have also done an explicit conversion to str if we wanted to keep using print - in this case, it was easier to just keep it as bytes.) [18:12:03] Ah, I see. Thanks for that. [18:12:11] I assumed for some reason the lines were too long or something [18:12:18] I don't remember now why I thought that [19:50:02] https://web.dev/isinputpending/ [20:44:53] Krinkle: can't see how it would work without being in both [20:55:22] AaronSchulz: you mean it should work fine if the new patch/branch is only on commons? [20:56:16] the state we had today was group0=wmf.18, group1: wmf.16 -> wmf.18, group2: wmf.16 [20:56:21] now rolled back to group0 [21:01:38] Krinkle: I made two branch patches now (one restoring the version with the key name bug and also the one fixing it) [21:02:08] that should get 16/18 on the same key scheme [21:02:40] (I suppose moving all to 18 would work too) [21:03:56] ack, but bypasses other people's changes having time to test [21:04:15] but hopefully ours was the only issue this week [21:31:17] AaronSchulz: rolling out now [21:31:23] s/now/soon [21:31:26] k [21:31:42] apparently CI is still at it [21:46:07] Krinkle: seems merged [21:47:06] ack, locking deploy now and wil stage now [22:43:45] AaronSchulz: (low prio) - do you know more about the history behind the file redirect caching and how important it is? I think mayve we should get rid of it, or at least make it limited to cases where we can't afford a quick db query. [22:44:08] like when doing a parser invocation, it seems bad to rely on that since there's no opppertunity to recover [22:44:17] there is one shot to purge it and then never again [22:44:33] it is indirectly backing File->exists() [22:44:39] which doesn't seem high traffic [22:45:00] I can look at it...I thought of simplifying that in that past if I recall [22:45:19] same for when we render the file description page, that's a single look up [22:45:27] seems like we can afford it there [22:45:47] but things get more complicated of course once non-db foreign repo is involved. [22:46:08] but in that case there's no shared cache, so not sure what we do there today actually, maybe no cache? or hopefully a short blind ttl [22:46:24] it seems more valuable there [22:46:46] I've got a good hour or two for anything now, something I can help with or CR? [22:47:33] https://gerrit.wikimedia.org/r/c/mediawiki/core/+/589465 ;) [23:13:51] AaronSchulz: ack, so to confirm again, we need this for the bagostuff stats, and more specifically we need bagostuff entry methods like set (not doSet) that take result of makeKey() as argument to be able to extract the key class. [23:14:43] yes [23:15:20] and the reason for not having the subclass implement only a "extractClassFromKey" method, is that the generalised aproach allows to re-encode full keys for the use case of proxy bags. [23:16:45] that and the fact that makeKey() might do hashing of large keys [23:18:36] AaronSchulz: ok. so should we expect in the non-proxy case that a typical instance like for apcu, memc or sql will involve general encoding, or would that not involve decoding/re-encoding? [23:19:06] I assume MemcBag->makeKey will do the specific encoidng, not the generic one, and that we don't intend to be able to decode that [23:19:23] Ah I see, the proxy class no longer proxies this call, it uses the generic one there [23:19:40] so we only expose generic encoding either from a proxy class, or if a specific medium doesn't need a more specific one. correct? [23:26:44] Krinkle: right [23:34:10] AaronSchulz: ok, I think I get it now. I got the impression that it was always using generic keys and sometimes transforming them as needed (such as memc), but that isn't the case. It also wasn't clear to me that proxyCall would only be used by wrapping bags, and not normally, and that the convert methods only work if called on or from an instance that opts in to generic keys. [23:34:12] LGTM :) [23:34:23] left a few nits but I don't expect much to change [23:37:24] cool