The class DeferredUpdates is using some services for database
Description
Description
Details
Details
Customize query in gerrit
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Open | None | T259960 Inject services into API modules and special pages | |||
| Resolved | DAlangi_WMF | T265749 Research to create service for DeferredUpdates::addUpdate() |
Event Timeline
Comment Actions
I've looked a bit into this. First of all, the DeferredUpdates class seems to be "ready" for servicification, in that the class itself already has well-defined responsibilities, methods have the proper visibility modifiers etc. Thus, creating a service from it would mostly be a matter of injecting some dependencies.
The only thing that needs a little bit of thought is how to handle the persistency of the state. Right now this is achieved with three static properties. The new service could either keep this behaviour, or become a singleton class. I don't have the knowledge necessary to pick an option here.
Either way, I think a general scheme might be to create a DeferredUpdatesRegistry (or Manager, or whatnot) with the same public interface as the current DeferredUpdates class, except not static. The DeferredUpdates class would simply proxy the calls to the new service, and become deprecated.
@aaron You're probably the best person to ask to. What do you think about this?
Comment Actions
It would be good to get cleanup patches like https://gerrit.wikimedia.org/r/c/mediawiki/core/+/596082 merged first.
See also https://gerrit.wikimedia.org/r/c/mediawiki/core/+/520954 for an early stab at this.
Comment Actions
Makes sense. I might review some of these cleanups, but I don't know that code enough to review the patch in its current form (i.e. as a single patch which does lots of things).
See also https://gerrit.wikimedia.org/r/c/mediawiki/core/+/520954 for an early stab at this.
Yeah, that seems similar to what I was thinking about. As a note, the queue properties were declared non-static there. While this is probably going to work (mostly) because of MediaWikiServices being a singleton, I think it wouldn't be bad to make them static, because we really don't want to lose any update.
Comment Actions
Change 709193 had a related patch set uploaded (by DannyS712; author: DannyS712):
[mediawiki/core@master] Add DeferredUpdatesManager service to replace DeferredUpdates
Comment Actions
Change 709197 had a related patch set uploaded (by DannyS712; author: DannyS712):
[mediawiki/core@master] Start injecting dependencies into DeferredUpdatesManager
Comment Actions
Change 709193 merged by jenkins-bot:
[mediawiki/core@master] Add DeferredUpdatesManager service to replace DeferredUpdates
Comment Actions
Change 709197 merged by jenkins-bot:
[mediawiki/core@master] Injecting dependencies into DeferredUpdatesManager
Comment Actions
This came up at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/938406, where it took me by surprise that we'd dependency-inject a DeferredUpdatesManager across the code base.
I this is worth re-evaluating based on:
- The needs and requirements of the DeferredUpdates feature.
- The cost of adopting it as a service.
- The benefits and met expectations of this utility as a service.
- The mistakes and unmet expectations of this utility as a service.
I believe DeferredUpdates generally is thought of as a process-level (i.e the current request as a whole) utility, much like built-ins like register_shutdown_function, header_register_callback, header(), and print (STDOUT). As well as logging/debugging utilities like wfDeprecated and Psr/Logger.
This is not a concept that we can turn into a service class without creating a large potential for developer confusion and mistakes. It is true, that at glance, a service class may look normal and familiar because we have so many service classes. However, sometimes looking different is needed in order to send the right message to developers about what it is. In this case, DeferredUpdates does not have to be its own special thing, it is merely a system utility like the others listed above. Not the most common, but not exactly uncommon either.
DeferredUpdates does not vary in a meaningful way by wiki configuration. It does have hard dependencies on interactions with other service classes right now. These could be addressed to become non-optional, or we could tolerate their use of the MediaWikiServices singleton as we do in Logger/MWDebug code.
What it is
Conceptually, DeferredUpdates(fn) either calls fn() immediately with nothing else special happening (when in a CLI/Test context), or it adds it to an array to be called at the end of the (global) request.
Maintainig these in the service container, implies the following:
- The behaviour is wiki-specific and configurable: No.
- It is safe to reset this service: No.
- It is reasonable to construct additional instances for another wiki: No.
- It is reasonable to construct multiple instances (by any means): No.
There must only be a single queue per global request, to ensure that updates remain linear, non-overlapping, atomic, in the expected order, and in the correct groups.
At the end of the global request, only the "main" instance from the "global" service container will have its updates executed. The rest is lost without warning, this is a problem.
I propose reverting most of this such that DeferredUpdates::addUpdate and other methods remain the primary non-deprecated entrypoint for this functionality (without service object or publicly reachable singleton methods).
Testing
In change 938406, @Daimona raises the very good point of testing code that (should) have no Database dependency. I offer two alternate ideas to consider:
- MediaWikiIntegrationTestCase could replace an (internally hidden) singleton with one that is database-agnostic.
- DeferredUpdates itself could be improved to compartmentalise its optional database features, such that they silenlty do nothing when the DB/storage services are disabled.
See also: T265398: Use DI for Profiler class instance in MediaWiki core code
Comment Actions
I'm proposing this task for the new team to use as an excercise to share knowledge and gain familiarity with DeferredUpdates. I'll be pairing with @xSavitar.
@DannyS712 I encourage you to stay involved as well if you like, keep an eye on future patches, share your knowledge, ask questions, review/suggest improvements, etc as you see fit.
Comment Actions
Change 949119 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):
[mediawiki/core@master] deferred: Move DeferredUpdates away from ServiceContainer approach
Comment Actions
Change 949989 had a related patch set uploaded (by Krinkle; author: Derick Alangi):
[mediawiki/extensions/CirrusSearch@master] tests: Make DeferredUpdates related test-suits integration tests
Comment Actions
Change 949989 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):
[mediawiki/extensions/CirrusSearch@master] tests: Make DeferredUpdates related test-suits integration tests
Comment Actions
Change 950205 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):
[mediawiki/extensions/AbuseFilter@master] Migrate DeferredUpdatesManager to use DeferredUpdates directly
Comment Actions
@Krinkle One idea that would make testing easier, and that could be perhaps a good compromise. Given its special nature, make DeferredUpdates a singleton (instead of a service), to guarantee that there's really a single instance around. But at the same time, keep its methods non-static, and introduce an interface for it (IDeferredUpdatesExecutor?) which provides methods such as addCallableUpdate and addUpdate. The singleton class would implement this interface, but we would also provide a lightweight implementation that runs the updates immediately (and perhaps one that discards them?). Code currently using DeferredUpdates statically would accept a new parameter, typed against the interface. I think would address your concerns:
- DeferredUpdates would not be a service
- There would be a single instance around
And at the same time, it would be possible to write unit tests for code using it.
Comment Actions
@Daimona If I understand correctly, the high-level outcome you'd like is to make it usable in pure unit tests (i.e. both storage and service container disabled). I agree that that is a desirable outcome.
I'm not sure about injecting it or making it configurable. I'd like to first explore if we can make it "just work" by letting the code (or TestCase base classes) automatically do the right thing. As originally designed the CLI mode should do this already, but a few complications creeped in that make this non-trivial today. I'm hopeful we can soften these dependencies.
Comment Actions
Indeed. This would be a major benefit.
I'm not sure about injecting it or making it configurable. I'd like to first explore if we can make it "just work" by letting the code (or TestCase base classes) automatically do the right thing. As originally designed the CLI mode should do this already, but a few complications creeped in that make this non-trivial today. I'm hopeful we can soften these dependencies.
Ah, yes, that could work too!
Comment Actions
Change 950206 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):
[mediawiki/extensions/CampaignEvents@master] Migrate DeferredUpdatesManager to use DeferredUpdates directly
Comment Actions
Change 949989 merged by jenkins-bot:
[mediawiki/extensions/CirrusSearch@master] tests: Make DeferredUpdates related test-suits integration tests
Comment Actions
@note: After the series of patches, DU depends on 3 services: StatsdDataFactory, DBLoadBalancerFactory and JobQueueGroupFactory. Will tackle disentangling each of them from DU piece by piece before moving forward on making tests (previous tests in extensions like AbuseFilter, CampaignEvents, and CirrusSearch) pure unit tests without the dependencies to any of the services mentioned above.
Comment Actions
Change 950209 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):
[mediawiki/core@master] deferred: Drop support for DeferredUpdatesManager
Comment Actions
Change 950205 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Migrate DeferredUpdatesManager to use DeferredUpdates directly
Comment Actions
Change 950206 merged by jenkins-bot:
[mediawiki/extensions/CampaignEvents@master] Migrate DeferredUpdatesManager to use DeferredUpdates directly
Comment Actions
Change 949119 merged by jenkins-bot:
[mediawiki/core@master] deferred: Move DeferredUpdates away from DUManager service approach
Comment Actions
Change 950209 merged by jenkins-bot:
[mediawiki/core@master] deferred: Drop support for DeferredUpdatesManager
Comment Actions
I like the idea of having DeferredUpdate act as a simple extension of the PHP runtime. However, its current implementation integrates with the per-wiki DB LoadBalancer instance. That logic would need to be factored out of the DeferredUpdate class. I added a comment on the patch with some ideas around that: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/951187/34#message-52b48595021e45b8f6c39f78a1bffd50feb88808
Comment Actions
Change 951187 had a related patch set uploaded (by Krinkle; author: Derick Alangi):
[mediawiki/core@master] deferred: First step toward DeferredUpdates decoupling
Comment Actions
Change 952288 had a related patch set uploaded (by Krinkle; author: Krinkle):
[mediawiki/core@master] deferred: Make DeferredUpdates docs more accessible
Comment Actions
Change 952271 had a related patch set uploaded (by Krinkle; author: Krinkle):
[mediawiki/core@master] deferred: Decouple DeferredUpdates from MediaWikiServices
Comment Actions
Change 952288 merged by jenkins-bot:
[mediawiki/core@master] deferred: Make DeferredUpdates docs more accessible
Comment Actions
Change 951187 abandoned by D3r1ck01:
[mediawiki/core@master] deferred: First step toward DeferredUpdates decoupling
Reason:
In favor of: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/952271
Comment Actions
Change 952271 merged by jenkins-bot:
[mediawiki/core@master] deferred: Decouple DeferredUpdates from MediaWikiServices
Comment Actions
All patches pending review has landed and the direction on how this is modeled has shifted. So we can call this resolved and create tickets to handle other aspects of DeferredUpdatesScope/DeferredUpdatesScopeStack.