[00:00:59] <TimStarling>	 AaronSchulz: you figured out what is going on there?
[00:02:54] <AaronSchulz>	 I see selectDB() handles failure poorly and that getDomainId() isn't updated. The whole "dbName" field should probably just be removed for being redundant.
[00:03:42] <AaronSchulz>	 (isn't updated in any case)
[00:05:27] <TimStarling>	 right
[00:06:01] <TimStarling>	 also schema and tablePrefix?
[00:07:02] <TimStarling>	 tablePrefix() at least does update the domain, but dbSchema() does not
[00:09:12] <TimStarling>	 looks like some of the subclasses have issues as well, such as DatabasePostgres::selectDB()
[00:12:37] <TimStarling>	 shouldn't there be a Database::selectDomain()?
[00:13:51] <AaronSchulz>	 selectDomain() would be more convenient for LB, yes
[00:16:18] <TimStarling>	 I don't understand how it would cause the bug though
[00:17:26] <TimStarling>	 reuseConnection() does use $conn->getDomainID(), but if $domain was wrong there, wouldn't it hit one of the sanity checks and throw an InvalidArgumentException?
[00:22:15] <TimStarling>	 ohh... openForeignConnection() in the reuse case does not use the input domain, $domainInstance, it uses the result of $conn->getDomainId() after selectDB() instead
[00:22:52] <TimStarling>	 that's why reuseConnection() does not throw, the connection is registered in LoadBalancer under the initial domain, not the current domain
[00:24:39] <TimStarling>	 so the "Reuse an in-use connection for the same domain" case would then cause the bug
[00:24:43] <AaronSchulz>	 yes
[00:25:18] <TimStarling>	 is there some way to have test cases for this?
[00:26:07] <AaronSchulz>	 selectDB/getDomainId() would be easy enough to test
[00:26:37] <TimStarling>	 we could test reuseConnection() with a mocked Database that keeps track of selectDB() calls
[00:26:41] <AaronSchulz>	 though it wouldn't be hard to test that LB case itself
[00:26:55] <AaronSchulz>	 there area already tests that get a few connections and do some domain stuff
[00:28:42] <TimStarling>	 LoadBalancer calls Database::factory(), there's a case in there which could almost be made to work for testing
[00:29:15] <TimStarling>	 if $dbType is not known it just does $class = 'Database' . ucfirst( $dbType );
[09:43:44] <tgr>	 legoktm: there are patches up for parsoid and MCS, not sure about changeprop
[09:44:00] <tgr>	 sorry, meant to ping Krinkle 
[14:03:28] <Reedy>	  includes/collation/data/first-letters-root.php                                       | 14747 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
[14:03:29] <Reedy>	 lol
[14:04:27] <Reedy>	 oh, it's a move
[14:10:55] <Amir1>	 Reedy: do you think you can review this patch? https://gerrit.wikimedia.org/r/c/mediawiki/core/+/452337 It's rather straightforward.
[14:12:30] <Reedy>	 I was going to say something about dry-run... But it's not really related
[14:16:47] <Amir1>	 thanks. I can work on the dry-run. What is it?
[14:17:20] <Reedy>	 I thought you were implementing a "dry-run" type feature, and we are trying to normalise maintenance scripts
[14:17:30] <Reedy>	 But it's not a dry run, just removing a potentially slow operation
[14:19:10] <Amir1>	 "we are trying to normalise maintenance scripts" <- That's interesting. Is there any notes I can read (I wrote some main. scripts, I want to make sure it follows decisions, etc.)
[14:20:27] <Reedy>	 Trying to remember...
[15:10:54] <Reedy>	 Travis is a bit broken
[16:20:47] <Reedy>	 16:20:05 Exception: ('command: ', "echo 'aawiki'; /usr/local/bin/mwscript update.php --wiki=aawiki --quick", 'output: ', 'aawiki\n#!/usr/bin/env php\nMediaWiki 1.32.0-alpha Updater\n\noojs/oojs-ui: 0.28.0 installed, 0.27.6 required.\nError: your composer.lock file is not up to date. Run "composer update --no-dev" to install newer dependencies\n')
[16:23:11] <Reedy>	 https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/452872/
[16:23:12] <Reedy>	 ffs jerkins