+1 for removing this.

@dpifke Do you want to take this on since you're working in this area?

Compression seems doable. LZMA works well per https://phabricator.wikimedia.org/T235455#5837382 . arclamp-grep would have to change though; maybe grep(fname, search_string) could stream zipped log object contents to lzcat and loop through the resulting lines.

This vaguely reminds me of https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/238370/ . Though implementing KeyValueStore makes more sense indeed.

Filed as https://github.com/phan/phan/issues/3706

I can reproduce this with master phan as well:

In T236880#5620053, @Anomie wrote:

+1, better documentation would be good. We've had a lot of complexity added to this code over the years.
As far as I can tell,

• getConnection: Basic "connect to a DB" functionality.
  • Of note is the fact that if you pass a non-false $domain you're supposed to call reuseConnection() before you let it go out of scope, which is easy to forget.
• getServerConnection: The differences from getConnection seem fairly well documented. The code in LoadBalancer clearly matches.
• getAnyOpenConnection: Not very well documented. At the least, this one won't open a new connection (returning null if no connection is already opened), which can be useful if you know you can skip some operation if nothing else already connected to the DB. But there may be other differences as well, the implementation seems completely separate from getConnection/getServerConnection.
  • If the only difference is actually the "no new connection" behavior, ideally the implementation should reflect that by passing a flag to some internal method shared with getConnection or getServerConnection.
  • If there are other differences, which seems likely, and those differences shouldn't be fixed, I suspect this shouldn't be on the interface at all.
    • The three external callers only use it to get a DB_MASTER for locking, so perhaps something that returns a proxy that only exposes the locking-related methods would serve to reduce confusion.
• getConnectionRef: Wraps the handle returned by getConnection with a proxy object that will automatically call reuseConnection() when it goes out of scope.
• getLazyConnectionRef: Like getConnectionRef but additionally won't even call getConnection until the first time it's actually used. Good if you think it often won't wind up being used after all.
• getMaintenanceConnectionRef: Like getConnectionRef but with the proxy implementing IMaintainableDatabase rather than just IDatabase.

Looks like hierdata/(swift|codfw)/params.yaml needs updating, along with the private puppet repo (beforehand).

Closing per the above patch (unless some issue remains).

I think having swift-repl manually set X-Timestamp is doable now. It would work kind of like rsync can in that regard. This also works better when the direction is switched. Right now, I assume the codfw files tend to have higher timestamps, so switching would cause pointless writes due to the new source cluster having higher timestamped files than the new destination cluster. Since the timestamp is already stored anyway, this wouldn't add any metadata.

In T244058#5851362, @Joe wrote:

In T244058#5849290, @aaron wrote:

Links to old (non-current) versions due not use the parser cache. This means that rendering will always require a full parse.

[CUT]

Some sort of parser caching could be considered for old links that get high traffic. We do not want to waste space on pcXXXX mariadb servers nor LRU flood memcached with large blobs, so there would have to be some "hotness" estimation logic, like a hitcounter.

Instead of caching, we should just rate-limit parsing of old revisions to N concurrent revisions per user or IP, probably via poolcounter.

I think https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/563289/ is running into this.

In T236800#5847650, @jijiki wrote:

@Krinkle we can push the new package to the canaries this week if you are ok with it

Links to old (non-current) versions due not use the parser cache. This means that rendering will always require a full parse.

I compressed a sample log file from today to see what kind of compression ratios we could get:

In T243619#5834657, @Anomie wrote:\

IMO we should never encourage use of query(), that's a code smell for probable MySQLisms.

In T243619#5834813, @Krinkle wrote:

In T243619#5834657, @Anomie wrote:

Not having conditions also implies not having a [...] determinstic outcome

How so? Our batching is generally for performance (i.e. avoiding locking the DB while updating millions of rows), as far as I know, not non-determinism.

Replication safety, determinism, query performance, and lock contention avoidance do often go hand-in-hand in my experience.
For a table like thing (id, foo, bar, baz) a write query that does not utilize a primary key, like UPDATE thing SET bar=42 WHERE bar=x AND baz=y is as I understand it considered an anti-pattern by our DBAs. In part because they are not replication-safe and indeed not (obviously) deterministic. I believe it would be a improvement to change such query to first SELECT the identifiers and then perform the write based by primary key. (Even without batching, although batching might make it even better still.)

Not seeing this in the logs anymore.

What user impact did it cause?

As long as there are any health checks that hit MediaWiki in codfw that involve DB access (pretty much any normal/special page view), then LoadMonitor::getServerStates is reachable (in the course of picking a DB to connect to). That seems expected to me.

In T183993#5801637, @Addshore wrote:

If they are more than one case ( I checked and it seems it's two cases in the past 7 days) We need to make sure tools or Wikibase itself put some time between edits (I think this is made using Wikibase itself) We should ask user what they used that made this happen.

Shouldn't chronology protector help us here?

These errors seem to be for Special:ConfirmEmail but the patch was for Special:ChangeEmail.

The RedisConnectionPool patch idea seems reasonable to me.

In T238493#5693277, @Gilles wrote:

@aaron have you started looking into this? Any leads?

In T230813#5648838, @Krinkle wrote:

In T230813#5599191, @Mholloway wrote:

Whoever picks this up, please ping me for access to the required private repo.

I thought maybe the ticket was missing some updates that were communicated elsewhere, but from what I understand our team was unable to install the extension. The review thus-far covered the backend and was based on static review of the code. Next steps is to figure out a minimal way to install (part of) it for frontend review.

Are there any cache busting user preferences at play here?

Aside from the things mentioned in the above patch, the overall code looks OK to me.

In T231086#5601608, @fgiunchedi wrote:

swiftrepl is puppetized now to run an eqiad -> codfw sync once a week on Monday (without deletes).

I just want it on the work board (I had a meeting with Erik/Bill) for tracking object cache review and work (we have the goal of getting CPT more involved in maintenance rather than just myself and Timo).

In T229686#5454294, @CDanis wrote:

In db-eqiad/codfw.php we currently provide IP addresses as the values of the keys in externalLoads instead of using hostsByName to translate hostnames to IP, e.g.:

'externalLoads' => [
	# es2
	'cluster24' => [
		'10.64.32.184' => 0, # es1015, C2 11TB 128GB, master
		'10.64.0.6'    => 1, # es1011, A2 11TB 128GB
		'10.64.16.186' => 1, # es1013, B1 11TB 128GB
	],
]

However looking at the code path, I don't think there's any reason why this has to be so. I see in LBFactoryMulti.php that newExternalLB calls newLoadBalancer which calls makeServerArray which implements the translation: $serverInfo['host'] = $this->hostsByName[$serverName] ?? $serverName;
So I'm planning on emitting hostnames when I implement externalLoads output in dbctl, but wanted to verify with @Krinkle and @aaron that I understood the situation correctly. @Krinkle also suggested that if we are going to rely on this behavior, then it should be explicitly tested in mediawiki-core.

In T224422#5563013, @jcrespo wrote:

So this is the latest news:
On T234948 we found that special page updates can lead to a constant 1-2 seconds of lag for the confluence of large long running query with heavy load from the many s8 edits + ongoing wikibase refactoring. Because the timeout is only 1 second, this means chronology protector will fail frequently. However, because we allow pooling for I think a limit of 5 seconds, the host is not depooled. Also, some jobs probably use the "slow" replica and try to use chronology protector, which leads to lots of errors. Obviously, increasing the timeout cannot be done lightly, as it could cause worse problems.
The good news is that I believe this was deployed successfully and works as intended. The bad news is that there is a weakness on our infrastructure, but I am not sure how to move forward, need suggestions. We can open discussion on another ticket, as this scope seems solved.

Indeed the logging is based on the *whole* raw unfiltered position...I should add a logstash key for the filtered one too.

@jcrespo @Marostegui What do think of the idea of having another cluster of mysql servers set up just like the parser cache ones? That would be nice from an HA perspective and to avoid adding extra load to any existing DB cluster (e.g. objectcache table of metawiki or extension1)? Traffic would be modest given that it would start out for use for WikimediaEvents, LoginNotify, perhaps AbuseFilter stats too (see https://docs.google.com/document/d/1tX8ekiYb3xYgpNJsmA1SiKqzkWc0F-_E4SGx6BI72vA/edit#heading=h.bdt9mhl3o7k5).

Not seeing this in the logs anymore.

In T157651#5535315, @kchapman wrote:

@aaron are you requesting code review from Core Platform or do you need something else?

Seems like some kind of merge conflict.

Odd, the constant seems to be there.

In T218207#5157583, @Krinkle wrote:

Interesting. In the specific case of SQLite, "cache in database" and "cache on disk" are effectively both use the disk. Some quick comparisons using Quick MediaWiki to install MediaWiki with SQLite (macOS, on-disk /private/tmp/quickmw, PHP 7.1.26 from Homebrew).

SQLite (default installation)

LocalSettings.php (generated)

$wgLocalisationCacheConf['storeServer'] = [
	'type' => 'sqlite',
	'dbname' => "{$wgDBname}_l10n_cache",
	'tablePrefix' => '',
	'variables' => [ 'synchronous' => 'NORMAL' ],
	'dbDirectory' => $wgSQLiteDataDir,
	'trxMode' => 'IMMEDIATE',
	'flags' => 0
];

Test: Deploy one language

time php maintenance/rebuildLocalisationCache.php --lang de --force
real	0m0.404s, 0m0.416s, 0m0.407s

! StaticArray

LocalSettings.php (appendix)

$wgCacheDirectory = $wgSQLiteDataDir;
$wgLocalisationCacheConf['store'] = 'array';

Test: Deploy one language

real	0m0.140s, 0m0.156s, 0m0.148s

Looks like Static Array beats SQLite as well. We've shown in all previous benchmarks that the "All languages" and "Page load time" use cases always align with the "One language" use case, so I won't bother re-running those. Besides, I don't think this would inform our decision here, as I don't think we should optimise the stock MW default for SQLite against MySQL and other RDBMS'es.
In the unlikely event someone finds that sqlite3-based writing or reading outperforms opcache-backed arrays, it will still work by default, and can be optimised by setting wgLocalisationCacheConf directly.

So, getting this test merged depends on redoing the wikibase schema hook application order for update.php. In CI, there seems to be a problem when it interacts with Flow hooks trying to make pages.

Should be fixed now.

It looks like WebStart.php sets ignore_user_abort() for POSTS and the major entry points have wfTransactionalTimeLimit() set for POSTS. In the case of module_deps updates for load.php, that's on GET.

Client disconnects (HTTP 499) are interesting...before the ignore_user_abort() in doPostOutputShutdown(), I suppose it's possible to end up with stuff like this (and long has been). https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/519741/ would help this particular case by avoiding DB writes.

In T231086#5434295, @CDanis wrote:

I wonder if some entry point lacks proper shutdown.

What is the value of apc.enable_cli ? I don't seem to have that problem.

I do worry about the risk of data loss if swiftrepl is also deleting files based on container list differences.

I'd love to have a simplified version of WebRequest as a service. One that would be useful for dealing with the issue that https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/532367/ is about. Optimization hacks like https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/526801/ could be avoided too. It could be injected with pathinfo/cookie settings, but would not deal with complex encoding stuff that uses $wgContLang and so on.

Still, a file was only uploaded, and no other operations done...I'm not sure why the DB would commit if the file store failed in one of the FileBackendMultiwrite backends and 'replication' is 'sync'...

Isn't there a swiftrepl background process to fix this?

Note that CdnCacheUpdate queues a purge to happen X seconds later to help deal with lag (mediawiki-config has $wgCdnReboundPurgeDelay at 11). If lag gets near that amount, then $wgCdnMaxageLagged will kick in.

In T229456#5417884, @Tgr wrote:

This is significantly less useful than the old behavior. Affected_rows is typically used to either skip expensive cache purges when nothing actually changed, or signal to the user whether they actually managed to change something. (Why would a caller care about rows matched but not changed?)

Seems to be resolved, likely by vary-revision refactoring from T226785.