[15:17:51] Looks like we hit T200000 in Phab. [15:17:52] T200000: Elaborate match Google user with e-mail address in addition to user ID - https://phabricator.wikimedia.org/T200000 [15:22:19] * greg-g throws a small quantity of confetti in the air so he doesn't have much to pick up later [15:23:20] greg-g: confetti are candy! you shouldn't throw candy around, you should eat it! [15:23:44] * greg-g throws it into his mouth [15:24:01] haha [15:25:00] AaronSchulz: re https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/446762/ if i get your idea right, then $conf['readOnlyReason'] would be used for push(), while wfConfiguredReadOnlyMode() would be used for pop() ? [15:28:13] mobrovac: right [15:28:24] greg-g: is there a deploy today (train)? [15:30:54] AaronSchulz: it was going to but there's a blocker: https://phabricator.wikimedia.org/T199941 [15:31:00] mobrovac: btw, how do we handle read-only mode? I guess it would have to re-enqueue since the queues are not per wiki, so there's no stopping (unless everything is read-only completely). [15:31:18] AaronSchulz: your quick take on that blocker (if it's worthy or not) more than welcome :) [15:31:24] greg-g: I'd add https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/446777/ to that list [15:33:24] AaronSchulz: is there a task for it? [15:34:08] no, Timo just mentioned it yesterday [15:34:10] AaronSchulz: ok, so conf['readonlyreason'] is derived in jobqueuegroup::singleton from wfConfiguredReadOnlyReason() and passed onto (and overridden) in JQG::get(), which creates consistency, if we were not to override it there, then one would need to set both the conf and wgreadonly to true in order to achieve the same effect, what my proposition does is that it leaves room for doing that too, all the while taking into account th [15:34:10] e fact that by default whether something can be enqueued is the property of the JQ in use (which would be neglected if done as you propose) [15:34:58] so i guess the question is: if we change it via conf[] for push and wfCROR() for pop(), then operators would need to change their configs [15:35:25] in the current proposition they don't, but they can override it if they wish to do so [15:36:25] AaronSchulz: when you have time, can you make a task so that we have something to track this? I don't know the "why" here :) [15:36:44] currently, there is no read-only mode in the sense that we count on at least one DC being able to process jobs, that said, if everything were to go read-only (which is at this point a manual endeavour), we'd jsut need to stop consumption from Kafka [15:39:54] mobrovac: why would it be neglected? If readOnlyReason was set in $wgJobTypeConf it would have effect (push and pop). [15:41:32] AaronSchulz: i meant that only setting wgReadOnly would be neglected if the operator doesn't also set conf['readOnlyReason'] [15:41:37] in our case, that is not relevant [15:41:48] but for e.g. JobQueueDB that is [15:42:32] hence, in the patch, for JQDB we have the default canEnqueue = false so that the behaviour and expectations don't change [15:43:01] since there it makes sense that if you are in read-only mode, then you wouldn't be able to even enqueue jobs [15:44:39] For the case of making everything read-only completely, the difference is either (a) setting canEqueue = false in codfw or (b) setting $wgJobTypeConf['default']['readOnlyReason'] = 'why'. As for JobQueueDB, it should probably subclass assertNotReadOnly() to check the LBFactory (even now, for sanity) [15:45:26] mobrovac: except we want to do exactly that in codfw. If possible, I want the semantics for us and third parties to be similar. [15:46:06] hm, in my view even if we go completely read-only we still can enqueue jobs [15:46:16] we just don't process them [15:46:24] *dequeue them [15:47:04] but i see your point [15:47:43] I'll create the alternative version [15:47:47] let's see then [15:47:48] :) [15:48:51] mobrovac: so this all goes back to the fact that we want to use $wgReadOnly for "this is a secondary DC that only takes reads to DBs, but lets jobs get pushed"? [15:50:14] essentially, yes, it goes back to the fact that we use wgReadOnly for things we shouldn't [15:50:17] it's like checking something that is *almost* want we want to check, hrm [15:50:42] basically, yes [15:51:15] so my patch goes in the direction that pushing jobs should be decoupled from executing them in the context of wgReadOnly [15:51:35] which, admittedly, is only a slight improvement over the current situation [15:55:08] I feel like we should change JobQueueGroup::get() not to override 'readOnlyReason' if isset(). Then we could make it controlled by a different etcd value. [15:55:43] there would still need to be another "disable pop" kind of field added to the classes [15:55:51] AaronSchulz: for the current problem, alternatively, we can just override assertNotReadOnly() in JobQueueEventBus because we don't use the pop() side of things, but that seems kind of avoiding the problem [15:56:26] AaronSchulz: yes, exactly, which is why i opted for canEnqueue (as the alternative would have been canDequeue or sth similar) [15:57:05] in reality, we should have both [15:57:20] and we shouldn't conflate read-only mode with either of these [15:57:37] but set either or both of these explicitly when in read-only mode [15:58:01] hm, that would create more maintenance burden [15:58:01] mobrovac: so 'canEnqueue' is just for third parties?\ [15:58:20] for third parties and for us in codfw :) [15:59:04] if 'readOnlyReason' only defaulted to $wgReadOnly but used any value explicitly provided, then that could just be 'false' in codfw [15:59:53] hm not really, because you still want to prevent writes to the db and job execution in codfw [16:00:11] which wgReadOnly is for [16:00:14] $wgReadOnly would be set for codfw [16:00:27] ah right right [16:00:30] sorry misread [16:00:32] but since readOnlyReason would be false (and the code would be changed not to clobber it), the jq would work [16:01:29] assertNotReadOnly would still need to use wfConfiguredReadOnlyReason [16:04:24] which means that we would still need to discern whether we are on the enqueue path or dequeue path [16:05:42] mobrovac: what for? I'm assuming there would be a per-DC etcd var for 'readOnlyReason', since that would be separate from $wgReadOnly. Are you thinking about pop()? [16:06:13] * AaronSchulz wonders if the DC/operation/$wgReadOnly combinations should be in a table somewhere [16:06:24] ah right, since we don't use pop() in our install, nothing would change [16:07:14] kk, let me write this alternative patch, sounds like the code might be much simpler than what we have now [16:07:21] we can make JQG::pop() check $wgReadOnly and such (I think JobRunner also does that already) if really wanted. That also would not effects us, since we never call pop(). [16:07:49] I should probably do the "default only" logic for FileBackend too [16:09:22] it only lets 'readOnly' override $wgReadOnly if set to a non-empty string, leaving no way to override it as true. A more versatile pattern is 'readOnly' ?? $wgReadOnly with the assumption that admins set 'readOnly' to either null, or a reason, or comment out the line setting it (in $wgFileBackends). [16:14:10] * AaronSchulz makes https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/446864 for filebackend [16:19:10] AaronSchulz: heh, it's a one-liner! https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/446866/ [16:21:12] greg-g: a made a task with the trace/patch [16:21:46] * AaronSchulz will look at the language one later [22:26:29] how are you meant to format a StatusValue? Status::wrap($sv)->get...()? [22:29:36] I think that's what most places do [22:37:03] seems like there should be a service for that