Page MenuHomePhabricator

MediaWiki should provide a LocalClusterObjectCache service
Closed, DeclinedPublic

Description

To deprecate $wgMemc (T160813) all existing usages should be migrated. There is no service to migrate too; $wgMemc calls ObjectCache::getLocalClusterInstance which has no service equivalent. Given that all that method does is pass $wgMainCacheType to the getInstance method of the same class, it could easily be a service instead, like has been done with getLocalServerInstance.

Event Timeline

Change 566083 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/core@master] Introduce a LocalClusterObjectCache service

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

Krinkle renamed this task from MediaWiki does not provide a LocalClusterObjectCache service to MediaWiki should provide a LocalClusterObjectCache service.Feb 3 2020, 4:00 AM
Krinkle moved this task from Library to Wiring for MediaWiki on the MediaWiki-libs-ObjectCache board.
CCicalese_WMF added a subscriber: CCicalese_WMF.

Untagging for now. Please feel free to re-tag Platform Engineering when it is ready for code review again.

Change 623114 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] WIP: inteoduce ObjectCache Manager

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

Change 623114 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] WIP: inteoduce ObjectCache Manager

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

This somewhat goes against the previous direction Aaron and I set up which is to phase out ObjectCache in favour of MediaWiki services directly.

  • WANObjectCache, LocalServerCache, and ParserCache are the ones we needed most and have been set up that way now.
  • LocalClusterCache was not done that way, because it's considered internal, but could be done still of course.

Are there others that we really need and wouldn't be obsolete or better suited by being injected explicitly into whatever needs it in service wiring? I've been thinking of "hash" "none" etc as more or less redundant/obsolete. I don't think we need those as singletons or services, but would be good to document the use cases if there are some.

See also:

I personally would appreciate having non-internal LocalClusterCache (aka APCu, not hash object). Without LocalClusterCache Wikibase wouldn't work in production (specially regarding property info and properties term store).

APCu is local "server" cache, and already available as public service.

The (internal) local" cluster" cache refers to the unwrapped Memcache/maincache as used by WANObjectCache, previously known as wgMemc. This is not used by Wikibase afaik.

The problem with LocalClusterCache is that there can be cycles or odd behavior for early calls. As long as the wiring works, then I'd love to have such a method.

APCu is local "server" cache, and already available as public service.

The (internal) local" cluster" cache refers to the unwrapped Memcache/maincache as used by WANObjectCache, previously known as wgMemc. This is not used by Wikibase afaik.

Wikibase uses ObjectCache::getLocalClusterInstance() for several services.

APCu is local "server" cache, and already available as public service.

The (internal) local" cluster" cache refers to the unwrapped Memcache/maincache as used by WANObjectCache, previously known as wgMemc. This is not used by Wikibase afaik.

Wikibase uses ObjectCache::getLocalClusterInstance() for several services.

OK. Can these be updated to use WANObjectCache? Or are there cases that functionally require bypassing the protections and statistics of WANObjectCache and/or are known to be incompatible with multi-DC set ups?

That would work but it seems some actually should be APCu and not memcached. I think it needs checking one by one.

I believe the reason Wikibase sometimes uses the local cluster instance is that WANObjectCache is a concrete class that doesn’t implement any useful interfaces, so by using it in a class you’re mandating a very specific caching strategy. Classes that use BagOStuff are more flexible and can have different kinds of caches injected into them (EmptyBagOStuff, HashBagOStuff, local server, local cluster, etc.) – but not WANObjectCache.

I believe the reason Wikibase sometimes uses the local cluster instance is that […] Classes that use BagOStuff are more flexible and can have different kinds of caches injected into them (EmptyBagOStuff, HashBagOStuff, local server, local cluster, etc.) – but not WANObjectCache.

I'm not sure I see the difference. WANObjectCache can wrap any BagOStuff object, so any BagO that you'd inject can be injected wrapped in WANCache just as well.

It offers the same level of abstraction, albeit through the common injection pattern rather than the subclass pattern. If it was a subclass this wouldn't work indeed, but it wraps, not subclasses.

The WANObjectCache service wraps the local cluster cache, so the two should be interchangeable for production code, and will always point to the same cluster.

function foo( BagOStuff $cache ) {
 
}
foo( new EmptyBagOStuff() );
foo( new HashBagOStuff() );

function bar( WANObjectCache $cache ) {
 
}
bar( new WANObjectCache([ 'cache' => new EmptyBagOStuff ]) );
bar( new WANObjectCache([ 'cache' => new HashBagOStuff ]) );
bar( WANObjectCache::newEmpty() );

Removing task assignee due to inactivity, as this open task has been assigned for more than two years. See the email sent to the task assignee on February 06th 2022 (and T295729).

Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome.

If this task has been resolved in the meantime, or should not be worked on ("declined"), please update its task status via "Add Action… 🡒 Change Status".

Also see https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator.

Change 566083 abandoned by Mainframe98:

[mediawiki/core@master] Introduce a LocalClusterObjectCache service

Reason:

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

function foo( BagOStuff $cache ) {
 
}
foo( new EmptyBagOStuff() );
foo( new HashBagOStuff() );

function bar( WANObjectCache $cache ) {
 
}
bar( new WANObjectCache([ 'cache' => new EmptyBagOStuff ]) );
bar( new WANObjectCache([ 'cache' => new HashBagOStuff ]) );
bar( WANObjectCache::newEmpty() );

Is this code allowed, though? WANObjectCache is not marked as @newable or @stable to call, and as far as I can tell, the stable interface policy doesn’t say that code in includes/libs/ is automatically stable without explicit annotations.

Edit: though I suppose the same is true of the BagOStuff classes too. I don’t know.

@LucasWerkmeister I don't think we've yet gone through the bagostuff library (incl WANObjectCache) to add the new stable interface policy markings as appropiate to reflect the undocumented/implied promise of stability we currently offer.

I believe de-facto we must treat this class as stable to call and newable because its constructor is the subject of $wgWANObjectCaches, which takes an object factory description. And indeed in general during past years, we do offer back-compat, deprecation and migration for changes to the WANObjectCache and BagOStuff constructors.

Having said that, it is unclear to me why you would need to construct a custom WANObjectCache instance in Wikibase. The instance from service wiring should suffice afaik, and we have no other backends in prod to my knowledge that it could be meaningfully used with. The default instance does, by default, automatically adapt to the configured local "main" cache of a given wiki.

For unit tests, it's of course fine to construct however you see fit. Either via WANObjectCache::newEmpty() or in a custom ad-hoc way if needed. I do note that we already replace these and others by default in MW's PHPUnit configuration with empty or hash bags for all integration tests.

Change 791038 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] objectcache: document BagOStuff and WANObjectCache as `@newable`

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

Change 791038 merged by jenkins-bot:

[mediawiki/core@master] objectcache: document BagOStuff and WANObjectCache as `@newable`

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