@daniel Ping

MY preferred solution would be to introduce a "ConnectionManager" or "ConnectionHandle" interface into core. WikibaseClient has ConsistentReadConnectionManager that would be one possible implementation. Setting the desired db groupcould easily be added to that.

Relevant code that currently takes a LoadBalancer or a DatabaseBase, or that calls wfGetDB or wfGetLB, would need to be changed to take a ConnectionManager.

I think this would be the cleanest solution. I have actually often wished for having such a class in core.

MY preferred solution would be to introduce a "ConnectionManager" or "ConnectionHandle" interface into core. WikibaseClient has ConsistentReadConnectionManager that would be one possible implementation. Setting the desired db groupcould easily be added to that.

This is very similar to my second idea… having it in core (in a widely used manner, with broad consensus) would probably make it a nice solution.

I'm not sure the gains outweigh the amount of work needed here, but then again, injecting DB connections, rather than using global functions sounds like a good idea to me in general.

One thing that is unclear: What are the benefits of introducing a new class rather than to extend LoadBalancer (which is what we use to "lazy" inject database connections these days).

Does getLazyConnectionRef() help here?

In T147169#2748912, @aaron wrote:

Does getLazyConnectionRef() help here?

Hm… I guess DBConnRef also kind of overlaps the idea behind the classes Daniel suggested: Wrap the database connection (getter) in a stateful way, so that we can attach further information to it (the default group to use, in this case).

We probably could use DBConnRef here.

Two things about DBConnRef that I don't like:

• It hogs the connection until it is destroyed. That makes me feel uncomfortable to make it a member in a service. Services should not own a connection permanently.
• If your service needs read- and write connections, you have to pass in two DBConnRef objects.

Regarding using LoadBalancer: we can, but then you need to pass in three arguments: the LB, the wiki (db) name, and the groups. The ConnectionManager would bundle that info (just like DBConnRef does).

I can see that… but it seems to me we're adding another layer just because the underlying ones are "bad", we should try to avoid that.

If the new thing allows us to use it as a replacement for passing LoadBalancer to encapsulate connections and for DBConnRef entirely, that would be a way forward, but just adding another thing sounds awry to me.

If the new thing allows us to use it as a replacement for passing LoadBalancer to encapsulate connections and for DBConnRef entirely, that would be a way forward, but just adding another thing sounds awry to me.

The new ConnectionManager thingy would be a replacement for passing LoadBalancer + wiki name + groups. It encapsulates the same information as a DBConnRef, but it would be different in some ways:

• it would not implement the DatabaseBase interface
• it would not hog a connection permanently.
• it would not automatically release anything when going out of scope.

To me, DBConnRef is a way to automatically release a connection after using it. It's basically a C++ style "automatic object" version of the Database object. So a DBConnRef's scope should be a single method invocation, it should not be a member. The new ConnectionManager could very well have a getConnectionRef() method in addition to a getConnection() method.

For the task at hand: to whatever is simple and gets the job done. If you don't want to mess with core, the simplest way would be to just pass the groups in addition to the wiki name. Using DBConnRef as a member without any modification is probably not a good idea, because it would hog the DB connection indefinitely.

Patch adding COnnectionManager to core: https://gerrit.wikimedia.org/r/#/c/322644/3

I looked into this some more just now:

I think the best way to go here is to make it possible (how?) to always append a "default" group to the $groups array in LoadBalancer::getConnection. That way, this group will be chosen over the "generic pool", whenever no group is explicitly asked for.

@hoo how necessary is this? Should we pick it up for Wikidata-Campsite?

In T147169#4257532, @Lucas_Werkmeister_WMDE wrote:

@hoo how necessary is this? Should we pick it up for Wikidata-Campsite?

Yes, we really want this

Change 440022 had a related patch set uploaded (by Hoo man; owner: Hoo man):
[mediawiki/core@master] Allow passing the default DB group to use in Maintenance scripts

https://gerrit.wikimedia.org/r/440022

Change 440022 merged by jenkins-bot:
[mediawiki/core@master] Allow passing the default DB group to use in Maintenance scripts

https://gerrit.wikimedia.org/r/440022

Change 440986 had a related patch set uploaded (by Ladsgroup; owner: Amir Sarabadani):
[operations/puppet@production] snapshot: make wikidata dump cronjobs use dump db servers

https://gerrit.wikimedia.org/r/440986

Change 441072 had a related patch set uploaded (by Hoo man; owner: Hoo man):
[mediawiki/core@master] Maintenance::finalSetup: Make sure we re-create LBFactory

https://gerrit.wikimedia.org/r/441072

Change 441073 had a related patch set uploaded (by Hoo man; owner: Hoo man):
[mediawiki/extensions/Wikibase@master] Make dump maintenance scripts use the "dump" db group

https://gerrit.wikimedia.org/r/441073

Change 441072 merged by jenkins-bot:
[mediawiki/core@master] Maintenance::finalSetup: Make sure we re-create LBFactory

https://gerrit.wikimedia.org/r/441072

Change 441073 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Make dump maintenance scripts use the "dump" db group

https://gerrit.wikimedia.org/r/441073

Change 441212 had a related patch set uploaded (by Hoo man; owner: Hoo man):
[mediawiki/extensions/Wikibase@master] Add "dumpDBDefaultGroup" repo setting

https://gerrit.wikimedia.org/r/441212

Change 441212 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add "dumpDBDefaultGroup" repo setting

https://gerrit.wikimedia.org/r/441212

Change 440986 merged by ArielGlenn:
[operations/puppet@production] snapshot: make wikidata dump cronjobs use dump db servers

https://gerrit.wikimedia.org/r/440986

@ArielGlenn I could confirm that this works correctly on the current dump run:

$ sudo -u dumpsgen sh -c "lsof -p `ps aux | grep dumpRdf.php | awk '{ print $2}' | paste -s -d ,`" | awk '/->db/ {print $9}'
snapshot1008.eqiad.wmnet:56086->db1087.eqiad.wmnet:mysql
snapshot1008.eqiad.wmnet:59318->db1087.eqiad.wmnet:mysql
snapshot1008.eqiad.wmnet:39248->db1087.eqiad.wmnet:mysql
snapshot1008.eqiad.wmnet:49804->db1087.eqiad.wmnet:mysql
snapshot1008.eqiad.wmnet:56006->db1087.eqiad.wmnet:mysql
snapshot1008.eqiad.wmnet:51752->db1087.eqiad.wmnet:mysql

I didn't get a chance to check things then but all open connections to dbs now from snapshot1008 are to db1087, as they should be. This will take some digging.

I will hazard a guess that the script initially retrieves the correct db for the dumps group but does not try to refresh it periodically (or on every failure). We could consider doing so after some number of consecutive failures; @hoo what do you think?

does not try to refresh it periodically

But db1101 never was a dump host, but rc, and it used to have load 1. Also I checked and didn't have any connected client (I checked).

In T147169#4498019, @jcrespo wrote:

does not try to refresh it periodically

But db1101 never was a dump host, but rc, and it used to have load 1. Also I checked and didn't have any connected client (I checked).

That's the most bizarre thing ever. All i can say is, we'll dig into it, and sorry!

No need to be sorry, could this be a load balancer bug?

In T147169#4498457, @jcrespo wrote:

No need to be sorry, could this be a load balancer bug?

I just looked into this briefly and skimmed all (seemingly) relevant code paths… I still have no idea how this could happen (also these errors don't seem to be on mwlog1001?!).

Does anyone have any further hints here?

I still have no idea how this could happen (also these errors don't seem to be on mwlog1001?!).

Does dumpJson.php call any function that could force any check on all replicas to fail (and because it has old configuration, it fails on a depooled non-slow host?

In T147169#4506139, @jcrespo wrote:

I still have no idea how this could happen (also these errors don't seem to be on mwlog1001?!).

Does dumpJson.php call any function that could force any check on all replicas to fail (and because it has old configuration, it fails on a depooled non-slow host?

I really can't think of anything like this. In the past MediaWiki used to "magically" do weird things like this, but I think it no longer does.

Having a quick look at what connections the dumpers currently have open also looks good to me:

hoo@snapshot1008:~$ sudo -u dumpsgen lsof -li | grep -P '('`ps aux | awk '/maintenance\/dump/ { print $2 }' | paste -d "|" -s`')' | grep -oP '(db|es)\d+\..*\.wmnet' | sort | uniq -c
      8 db1104.eqiad.wmnet
      4 es1011.eqiad.wmnet
      4 es1013.eqiad.wmnet
      6 es1014.eqiad.wmnet
      2 es1019.eqiad.wmnet

I will also log this information (in a screen on snapshot1008) for the next hours/days and see whether any unexpected connections are opened.

@jcrespo Do you have any more information for what to look out here?

@jcrespo Do you have any more information for what to look out here?

The problem is most likely the loadbalancer- working with old data, even if the server is not needed, generates lag warnings for some complex dependency of how lag check works (generating 40K logs per minute). Abandon ship- this is not going to work, and nothing that wikibase developers or DBAs do can fix it.

Although you could do a new database requests every 10 minutes or so to mitigate that. Kill the process and restart it so it reloads the new config.

In T147169#4666218, @jcrespo wrote:

@jcrespo Do you have any more information for what to look out here?

The problem is most likely the loadbalancer- working with old data, even if the server is not needed, generates lag warnings for some complex dependency of how lag check works (generating 40K logs per minute). Abandon ship- this is not going to work, and nothing that wikibase developers or DBAs do can fix it.

Although you could do a new database requests every 10 minutes or so to mitigate that. Kill the process and restart it so it reloads the new config.

We currently run the dumper instances for about 1h... would it be helpful to shorten this to maybe 30m?

In T147169#4669702, @hoo wrote:

In T147169#4666218, @jcrespo wrote:

@jcrespo Do you have any more information for what to look out here?

The problem is most likely the loadbalancer- working with old data, even if the server is not needed, generates lag warnings for some complex dependency of how lag check works (generating 40K logs per minute). Abandon ship- this is not going to work, and nothing that wikibase developers or DBAs do can fix it.

Although you could do a new database requests every 10 minutes or so to mitigate that. Kill the process and restart it so it reloads the new config.

We currently run the dumper instances for about 1h... would it be helpful to shorten this to maybe 30m?

It will definitely help, but it may still get into race conditions, but obviously less than if it is every 1h :-)

In T147169#4669703, @Marostegui wrote:

In T147169#4669702, @hoo wrote:

We currently run the dumper instances for about 1h... would it be helpful to shorten this to maybe 30m?

It will definitely help, but it may still get into race conditions, but obviously less than if it is every 1h :-)

I'll look into this :)

I found no connections from our dump scripts to any db other than db1104 (and some es hosts).

Change 468059 had a related patch set uploaded (by Hoo man; owner: Hoo man):
[operations/puppet@production] Wikidata entity dumps: Halve pagesPerBatch

https://gerrit.wikimedia.org/r/468059

Change 468059 merged by ArielGlenn:
[operations/puppet@production] Wikidata entity dumps: Halve pagesPerBatch

https://gerrit.wikimedia.org/r/468059

The above script should be in effect for the rdf jobs this week and for all wikidata weeklies after that.

@hoo is this done now? If so can we close it? :)

The patch that was pushes out cuts down the amount of time that a script may have failed to notice a db configuration change, but it doesn't address the root cause, which is rather harder.

Not sure if entirely related to the scope of this task but, please see: T138208#6809748

These are for the weekly wikidata "entity dumps", and so separate from the main xml/sql dumps implicated in the other task.