| Jdforrester-WMF | |
| Apr 16 2026, 4:15 PM |
Follow-up from T423311.
The deployment-charts define MCROUTER_SERVER as a string that's "<host>:<port>", which means we need to split out the two parts we're currently using explode(':',…) in the hot path (mc.php), and it is a footgun for when the server turns to be IPv6.
Let's instead consider providing MCROUTER_HOST and MCROUTER_PORT (or whatever), with the current users of MCROUTER_SERVER possible using the very-cheap string-addition in the hot path instead?
Change #1275465 had a related patch set uploaded (by RLazarus; author: RLazarus):
[mediawiki/extensions/WikiLambda@master] MemcachedWrapper: Accept server config key, deprecate host and port
Change #1275467 had a related patch set uploaded (by RLazarus; author: RLazarus):
[operations/mediawiki-config@master] mc: Set server, instead of host and port, for wgWikiLambdaObjectCaches
Change #1275468 had a related patch set uploaded (by RLazarus; author: RLazarus):
[mediawiki/extensions/WikiLambda@master] MemcachedWrapper: Drop support for deprecated host and port config
I agree that we should take the explode call out of the hot path, but reworking the existing env variables is probably more than we want to tackle -- we could orchestrate the config change for ourselves, but it'd also be a visible change for non-WMF users of MediaWiki with memcache. Not impossible but a lot of work, especially if we don't end up keeping MemcachedWrapper in the long run.
The existing $MCROUTER_SERVER is plumbed through to MemcachedPeclBagOStuff's initializeClient(), with some regexes that perform basically the same function as the explode call. My alternate suggestion is to use the same regexes (that gets us IPv6, and $MCROUTER_SERVER compat generally) in the MemcachedWrapper constructor rather than mc.php (that gets it out of the hot path). That's up for review (code change 1, config change 2, code change 3).
The duplication isn't my favorite -- if we do keep MemcachedWrapper permanently, we could either share the logic more formally on the two codepaths, or revisit a separated HOST and PORT env interface for both of them. (In that case we might prefer to plumb that change all the way through to MemcachedPeclBagOStuff, rather than joining the strings just to re-split them.) But from a performance standpoint, these are fast regexes on short strings, and once it's moved into our constructor so that we're not hitting it on every request, I'm not super worried about it.
@Jdforrester-WMF WDYT? Coments welcome on those patches if the basic approach sounds good, or here if you'd rather take a step back and go a different way.
Change #1275465 merged by jenkins-bot:
[mediawiki/extensions/WikiLambda@master] MemcachedWrapper: Accept server config key, deprecate host and port
Change #1275952 had a related patch set uploaded (by Jforrester; author: Jforrester):
[mediawiki/extensions/WikiLambda@master] tests: Add direct coverage of MemcachedWrapper
Change #1275952 merged by jenkins-bot:
[mediawiki/extensions/WikiLambda@master] tests: Add direct coverage of MemcachedWrapper
Change #1275467 merged by jenkins-bot:
[operations/mediawiki-config@master] mc: Set server, instead of host and port, for wgWikiLambdaObjectCaches
Change #1275468 merged by jenkins-bot:
[mediawiki/extensions/WikiLambda@master] MemcachedWrapper: Drop support for deprecated host and port config