Page MenuHomePhabricator

Provide a narrow interface for code that needs to wait for DB replication lag
Open, NormalPublic

Description

Code that does lots of DB writes and needs to wait for DB replication lag must either call the deprecated wfWaitForSlaves() method or pull in LBFactory; the first goes against dependency injection principles, the second adds more dependencies and complexity than needed. There should be a narrower interface (whether it's implemented by LBFactory or not) for waiting for replicas to catch up.

Event Timeline

Tgr created this task.Sep 3 2018, 6:34 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 3 2018, 6:34 PM

It's unclear to me what problem this is aiming to solve.

Would "code that does lots of DB writes" not naturally hold an LBFactory object regardless?

How could this "replication manager" interface be realistically be implemented by anything other than LBFactory? Hence, what is the added value of the distinction, if we were to make that?

Restricted Application added a project: Core Platform Team. · View Herald TranscriptJul 23 2019, 6:14 PM
Krinkle triaged this task as Normal priority.Jul 23 2019, 6:14 PM
Anomie moved this task from Inbox to Icebox on the Core Platform Team board.Jul 23 2019, 6:21 PM
Anomie added a subscriber: Anomie.

DI advocates like to have such single-method interfaces. The claimed benefit is that the resulting code doesn't actually "depend" on the whole of LBFactory even though LBFactory is needed to implement the interface.

Tgr added a comment.Jul 23 2019, 6:41 PM

Application code typically needs three things from the DB layer: a way to get connection objects, a way to defer slow queries during performance-sensitive operations and a way to ensure long-running queries do not cause disruption. Historically, the first was done via wfGetDB, the second via DeferredUpdates, the third via wfWaitForSlaves. With dependency injection, the first is done via injecting lazy DB handles, the second will presumably be done by turning DeferredUpdates into a service, but there's no obvious path forward for the third. Se we end up injecting LBFactory, which is an obvious violation of the interface segregation principle: it's a complex interface with very little cohesion and lots of random staff in it that pretty much no caller ever cares about (setTableAliases? getChronologyProtectorClientId? flushReplicaSnapshots? rollbackMasterChanges? etc).

The implementation would presumably be by the replication manager holding the LBFactory instance and proxying calls to it (the traditional way would be to split ILBFactory into smaller pieces, but per T193613: Establish stable interface policy for PHP code (was: strategy for PHP interface changes) we are trying to move away from relying on interfaces). Whether LBFactory itself should be split up into smaller classes or not is not relevant for the purposes of this task (and I'm not familiar enough with the code to have an opinion on it).