The user needs movestable or review. The code could just be changed to also allow moves with autoreview.

An example of a diff where lines are added is https://en.wikipedia.org/w/index.php?title=User:Spicy/spihelper_log&curid=74418657&diff=1216025285&oldid=1216024788

In T221795#9666191, @Ladsgroup wrote:

FWIW, this system has caused an incident where several million articles were not editable because this process locked 3M rows in pagelinks. Fixing that would improve our resilience.

For basic maintainability, no new "support" should be added for this "feature".

I think the intent was to have better consistency for doOperations() calls with only a single-step file operation (e.g. not move for swift). If a PUT request fails in the master backend, then not trying it on the second backend keeps things consistent. This doesn't cover the edge case of a non-answer (e.g. timeout or 503) where the objects where ultimately saved.

In T348294#9593024, @Bawolff wrote:

When i was testing this, the new version did still calculate some SHA1's in maybeUpgradeRow in a deferredupdate. However that is unrelated to file backend, and possibly is something about my local install or the file type handler.

I couple things I wonder about:

• Though the bottleneck seems to be EventGate more than Kafka, I still wonder why profile::kafka::mirror::properties doesn't blacklist all MW jobs?* Is anything making use of that extra data?
• Are there stats on the average byte length of jobs enqueued? Maybe JobQueueEventBus could emit those to find bulky jobs. At least cirrus search jobs are known for being bulky. Maybe there are more. I assume the bulky cirrus job problem will be resolved if the new stream-based updater is deployed (T317045).
• I wonder if JobQueueGroup::lazyPush()/JobQueueEventBus could be rigged to make the provided jobs use "hasty" mode in EventGate?

I think having fatals and warnings is useful and reasonable. What seems confusing now is that we have kind-of-three levels of errors, one of which is confusingly called an "error" type error:

• StatusValue::warning() sets a "warning" error.
• StatusValue::error() sets an "error" error. This seems the most dubious. Is it supposed to mean that something definitely failed, but didn't ruin the whole operation? I can't see where this would come up besides some kind of very unusual sub-status merging, which should probably be done with custom merge logic anyway (possibly changing sub-status fatals to warnings).
• StatusValue::error() sets a "error" error (not a "fatal" error), and makes isOK() return false. Since the type is just "error", one can no longer find "which errors" caused the fatal (if any, since something could just call setOK( false ) for some reason).

I don't think anything new should get built on top of $wgSharedTables or setTableAliases(). Those are basically old tech debt that's akin to inserting things in the middle of a jinga tower to hopefully get the intent result.

In T313731#9498030, @Ladsgroup wrote:

Yes, That's how other pruning of old rows works in mediawiki (RC, CU, etc.) and for the sake of simplicity of architecture, it should just reuse the same concepts if not the same code.

In T313731#9488510, @Ladsgroup wrote:

The circular replication is quite a snowflake and has caused issues before, I'd avoid it if possible.

I'm strongly in favor of getting rid of the mess that is $wgSharedTables. Any mechanism that supports sharing tables needs to be designed to be aware of that, using appropriate methods to get database handles that are documented as being on possibly different databases. The caller would at least know that certain tables are co-located.

I'd rather not have the writes be cross-datacenter, tying up the working thread in the post-send stage of web requests, especially given the potential for spikes. If there was some aggregation service in the middle (batching/flushing counter updates), that would less risky, though more complex.

Is this still an issue? I'm not seeing this in any kibana production logs.

One last point was that the configuration arrays include a 'db' key, which is actually a DB Domain. Should 'db' be enforced as a database name only (not a full DB Domain), maybe renamed 'dbname', or should it be renamed 'dbDomain', or maybe something else?

What about VirtualDomainsMapping vs VirtualDomainMapping? Maybe it could even be DatabaseVirtualDomainMapping since this has nothing to do with vhosts or anything like that?

So, without T246371 being done yet, it looks like current apache config on the jobrunners won't let you use RunSingleJobHandler, but only /rpc/* stuff. So, either:

• an exception would have to be made for the /w/rest.php/eventbus/v0/internal/job/execute within modules/profile/templates/mediawiki/jobrunner/site.conf.erb
• rpc/RunSingleJob.php would have to temporarily be used by the search updater and migrated later.

One thing to also fix here is that things like SELECT FOR UPDATE, SELECT GET_LOCK()...any SELECT really...should be exempted from the transaction size check in approvePrimaryChanges(). There is no use in ROLLBACK at that point. I'll make a patch for that.

Re-reading the description and comments it seems to me that this task is more about addQuotes(), phan-taint, and a more fluent looking interface more so than eliminating raw SQL from code calling rdbms query methods. Is the idea to keep most of the build*() methods around? Currently, the $field arguments to Database::expr() allows raw SQL in $field. Is that intentional in the long run? If so, then buildMultiUpsertSetForOverwrite() can just use SUBSTR() in the $field argument. SelectQueryBuilder::fields() allows raw SQL fields and SelectQueryBuilder::where() allows raw SQL. On the other hand, SelectQueryBuilder::table() wants a query builder for computed/derived tables rather than a string from selectSQLText() or such.

Maybe SyncObjectStash is an OK name?

I mean that the job would never be enqueued into kafka, it would be executed by specifying the serialized job to RunSingleJobHandler, which runs it on the fly and returns a status code. If retries are enabled, it just changes the response status in some cases, but that endpoint itself does not try to re-enqueue the job (in this case a never-enqueued job). The Job class would decide what success/error info is propagated up to the JSON response. None of this would involve touching kafka. The stream updater would just be POSTing requests to the RunSingleJobHandler to "refresh this page" and reading the response.

I feel like the current MainStash could equally or more so be called a WANObjectStash that a one that routes to a single datacenter. Essentially, the main stash now is one that must perform in multiple DCs, sacrificing more consistency in order to do so. It's tricky to come up with a good name though. Maybe:

• ColocaleStash
• OneRegionStash
• LinearStash (e.g. best-effort tries to be linearizable for single-key operations)

It looks like the core MW ApiQuerySearch module does not require POST and the HTTP body parameter in the linked code don't seem like they would be too long, even 'gsrsearch' which is basically a just title AFAIK. So, this looks like it's a matter of changing the restbase call to GET and moving the body parameters to URL parameters. The larger work probably lies in setting up and testing restbase.

If T246371 was done, then the stream updater could just make a request to run a serialized job (provided in the same request) for each page that needs updating. The job would do the backend work of prop=cirrusbuilddoc and also the search index updates. Though the job would have to be registered in $wgJobClasses, nothing would actually enqueue the jobs. The error/retry logic and ability to curtail concurrency would be controlled by the stream updater rather than the generic job runner logic. Would that avoid the job queue issues?

In T210206#9312960, @Ladsgroup wrote:

@daniel @matmarex @Krinkle the LIKE expression implementation gave me an idea for cases of ipb_range_end = ipb_range_start in WHERE conditions and possibly join conditions. We could introduce a RawValue class and turn ipb_range_end = ipb_range_start into $dbr->expr('ipb_range_end', '=', RawValue( 'ipb_range_start' ) ) instead. It's not pretty but it's clearly not a common usecase. Thoughts?

Basically flipping the default of "raw SQL unless specified" to "quoted unless explicitly specified"

In T345185#9141119, @Tgr wrote:

I would maybe create a small authentication extension that uses PHP configuration to define a map of bearer token => (username, MWRestrictions, grants[]).

I don't think API Platform will take this up anytime soon, but I wouldn't mind helping with review.

In T247552#9300635, @Raymond wrote:

In T247552#9296703, @aaron wrote:

CC'ing @Nikerabbit . The new messages can clone the old translations. I don't know the exact TWN process for that, but it should apply here.

@aaron In case you have to move a message from one repo to another repo translatewiki scripts can move the translations to the new repo and keep the translation history. No need to duplicate the translations.

In case a message will be kept in the old repo and added with another message key in another repo we do not have a script to duplicate translations. Please add the translations in the 2nd repo.

Add me as i18n reviewer in both cases. Thanks.

In T314908#9296903, @Tgr wrote:

• We add a prefix option to VirtualDomainsMapping (alongside cluster / db). The default configuration uses a prefix instead of a separate DB; small-scale MediaWiki users are encouraged to use prefixes instead of separate DBs for their wiki farms. Not sure if this would cause an issue with PHPUnit tests which use a custom DB prefix for the test database (but then PHPUnit can't be used to test cross-wiki features anyway).

In T349761#9293600, @Tgr wrote:

That said, as long as we go with a single prefix, virtual- is the logical one for database domains that are differentiated from normal DB domains by being virtual. I'd really like to consider what @aaron said in T330590#9260204, though - we'll need to somehow differentiate between virtual domains that are there to share tables between multiple wikis in a wiki farm and those which only exist as an idiosyncratic sharding mechanism. It could be done via documention, but using different prefixes for those two purposes seems like a nice way to do it.

CC'ing @Nikerabbit . The new messages can clone the old translations. I don't know the exact TWN process for that, but it should apply here.

I think we should nail this down before things get backed into stable releases and compatibility is hard to break.

That sounds consistent with the operations failing in one data-center (the closest to the uploader) and then getting cleaned up by the periodic sync script.

In T341007#9251361, @Beao wrote:

Am I experiencing the same problem?

I'm getting this error using the normal uploader at Special:Upload. After trying to upload, Special:Upload displays a red box saying "An unknown error occurred in storage backend "local-swift-eqiad"." and sometimes the uploaded file is replaced by the original file while the original file disappears.

I've compiled a gallery at https://commons.wikimedia.org/wiki/User:Beao/Images_with_upload_error

I wonder if the auth token just expired while the combined file was being uploaded (but after the PUT started). By the time the chunk deletions are issued, perhaps the token expired. AFAIK, the tokens should last for a day and MediaWiki app servers only reuse them for ~15 minutes (which used to be higher, but they seemed to expire too soon). It would be interesting to see the memcached stats for the token storage on the swift servers (maybe there is eviction pressure for some reason, like some service never reusing tokens and thus flooding memcached).

I'm noticing that GroupPermissionsLookup is poorly documented.