Page MenuHomePhabricator
Create Task
Maniphest T299691

Break down monster class: Database
Closed, ResolvedPublic
Actions

Description

This class has more than 6000 lines and more than 100 public functions, it does everything related to databases and needs to be broken down. Some ideas that were brought up by @daniel are:

  • Migrate transaction management to its own class and interface (T299698)
  • Move replication-based stuff to its own class, e.g. getLag()
  • Remove legacy cruft from introduction of ResultWrapper: T286694
  • Move schema update and related functions to IMaintainableDatabase or DatabaseUpdater (T190396)
  • Move SQL building code from Database class to SQLPlatform (T307616)

Details

SubjectRepoBranchLines +/-
rdbms: avoid DB_PRIMARY queries in getLagFromPtHeartbeat()mediawiki/coremaster+41 -60
rdbms: Introduce DatabaseFlagsHolder and move some internal logic theremediawiki/coremaster+261 -168
rdbms: Moving replication-related code to its own componentmediawiki/coremaster+775 -727
rdbms: Move Database::factory() to DatabaseFactory servicemediawiki/coremaster+308 -155
rdbms: remove getTopologyRootPrimary() and clean up related fields/parametersmediawiki/coremaster+158 -125
rdbms: Move several methods from IDatabase to IMaintainableDatabasemediawiki/coremaster+39 -35
rdbms: Start using SQLPlatform and move more methods theremediawiki/coremaster+224 -125
rdmbs: Start of SQLPlatform to split out of Databasemediawiki/coremaster+402 -121
Customize query in gerrit

Related Objects
Search...

Event Timeline

Ladsgroup created this task.Jan 20 2022, 7:44 PM
Restricted Application added projects: Performance-Team, Platform Engineering. · View Herald TranscriptJan 20 2022, 7:44 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Ladsgroup updated the task description. (Show Details)Jan 20 2022, 7:45 PM
Ladsgroup added a subtask: T286694: Drop legacy cruft arising from introduction of ResultWrapper.
Izno subscribed.Jan 20 2022, 8:56 PM
LSobanski moved this task from Inbox to Epic - Database on the Data-Persistence board.Jan 21 2022, 9:27 AM
• dpifke edited projects, added Performance-Team (Radar); removed Performance-Team.Jan 24 2022, 7:38 PM
• dpifke moved this task from Limbo to Watching on the Performance-Team (Radar) board.
Ladsgroup mentioned this in T297633: <Tech Initiative> Improving Databases in MediaWiki.Jan 24 2022, 8:02 PM
aaron subscribed.Jan 24 2022, 11:55 PM

The "Move replication-based stuff to its own class" bullet could use some clarity. I'd imagine a lot of that could easily stay in Database/subclasses (due to rdbms specific logic). I guess you could have separate per-rdbms class hierarchies and inject the objects. Was this referring to more of the LoadBalancer code?

Ladsgroup added a comment.Jan 25 2022, 12:46 PM

Yes, you basically answered yourself. This needs to be per-rdbms replication manager and we also need to move LB and LBFactory logic to it (as much as possible) as well, that would make the class more conceptually cohesive.

Ladsgroup updated the task description. (Show Details)Jan 26 2022, 6:16 PM
Ladsgroup updated the task description. (Show Details)Jan 26 2022, 6:39 PM
Krinkle moved this task from Untriaged to Rdbms library on the MediaWiki-libs-Rdbms board.Jan 26 2022, 8:16 PM
Ladsgroup closed subtask T299698: Move out transaction management out of Database class as Resolved.Mar 7 2022, 4:28 PM
Ladsgroup closed subtask T286694: Drop legacy cruft arising from introduction of ResultWrapper as Resolved.Mar 15 2022, 8:32 PM
RhinosF1 subscribed.Mar 15 2022, 9:32 PM
Krinkle mentioned this in T305016: Think about rdbms reconnection logic.Mar 30 2022, 12:29 AM
Krinkle triaged this task as Medium priority.Apr 7 2022, 6:52 PM
Krinkle added a subtask: T302953: Deprecate wasDeadlock/wasLockTimeout/wasConnectionLoss as public IDatabase methods.
gerritbot added a comment.Apr 12 2022, 1:25 AM

Change 745215 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] rdmbs: Start of SQLPlatform to split out of Database

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

gerritbot added a project: Patch-For-Review.Apr 12 2022, 1:25 AM
gerritbot added a comment.Apr 24 2022, 4:26 PM

Change 745215 merged by jenkins-bot:

[mediawiki/core@master] rdmbs: Start of SQLPlatform to split out of Database

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

Maintenance_bot removed a project: Patch-For-Review.Apr 24 2022, 4:30 PM
ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.9; 2022-04-25).Apr 24 2022, 5:00 PM
gerritbot added a comment.Apr 25 2022, 8:55 AM

Change 785807 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] rdbms: Start using SQLPlatform and move more methods there

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

gerritbot added a project: Patch-For-Review.Apr 25 2022, 8:55 AM
Krinkle added a subtask: T294969: Deprecate and remove Database::lockTables and Database::unlockTables.May 4 2022, 5:15 PM
Krinkle closed subtask T294969: Deprecate and remove Database::lockTables and Database::unlockTables as Resolved.
gerritbot added a comment.May 4 2022, 6:56 PM

Change 785807 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Start using SQLPlatform and move more methods there

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

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.12; 2022-05-16); removed MW-1.39-notes (1.39.0-wmf.9; 2022-04-25).May 4 2022, 7:00 PM
Maintenance_bot removed a project: Patch-For-Review.May 4 2022, 7:30 PM
Krinkle added a subtask: T314094: Deprecate and remove IDatabase::lastQuery().Aug 1 2022, 6:57 PM
Ladsgroup closed subtask T307616: Move SQL building code from Database class to SQLPlatform as Resolved.Aug 2 2022, 8:56 PM
Jdforrester-WMF updated the task description. (Show Details)Aug 5 2022, 8:34 PM
Jdforrester-WMF updated the task description. (Show Details)
Jdforrester-WMF updated the task description. (Show Details)
Jdforrester-WMF updated the task description. (Show Details)
Krinkle added a subtask: T190396: Consider splitting the IDatabase interface.Aug 6 2022, 2:34 AM
Krinkle closed subtask T190396: Consider splitting the IDatabase interface as Resolved.Aug 6 2022, 2:38 AM
Krinkle mentioned this in T190396: Consider splitting the IDatabase interface.
Krinkle merged a task: T190396: Consider splitting the IDatabase interface.
Krinkle added subscribers: Addshore, gerritbot, Jakob_WMDE and 2 others.
Krinkle added a comment.Aug 6 2022, 2:41 AM
In T190396#8135052, @Krinkle wrote:

A summary of the work so far:

Two major classes have been split out from IDatabase by @Ladsgroup over the past few months:

This in addition to removing (not moving) various methods such as:

gerritbot added a comment.Aug 8 2022, 11:03 AM

Change 821201 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] rdbms: Move several methods from IDatabase to IMaintainableDatabase

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

gerritbot added a project: Patch-For-Review.Aug 8 2022, 11:03 AM
gerritbot added a comment.Aug 8 2022, 2:32 PM

Change 821201 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Move several methods from IDatabase to IMaintainableDatabase

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

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.25; 2022-08-15); removed MW-1.39-notes (1.39.0-wmf.12; 2022-05-16).Aug 8 2022, 3:00 PM
Maintenance_bot removed a project: Patch-For-Review.Aug 8 2022, 3:31 PM
Ladsgroup updated the task description. (Show Details)Aug 15 2022, 7:59 AM
gerritbot added a comment.Aug 19 2022, 11:51 AM

Change 824717 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] [WIP] [VERY WIP] Moving replication-related code to its own component

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

gerritbot added a project: Patch-For-Review.Aug 19 2022, 11:51 AM
gerritbot added a comment.Aug 19 2022, 12:18 PM

Change 824722 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] rdbms: Introduce DatabaseFlagsHolder and move some internal logic there

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

gerritbot added a comment.Aug 24 2022, 1:10 AM

Change 825930 had a related patch set uploaded (by Aaron Schulz; author: Aaron Schulz):

[mediawiki/core@master] rdbms: remove unused getTopologyRootPrimary() method from IDatabase

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

gerritbot added a comment.Aug 29 2022, 1:37 PM

Change 825930 merged by jenkins-bot:

[mediawiki/core@master] rdbms: remove getTopologyRootPrimary() and clean up related fields/parameters

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

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.27; 2022-08-29); removed MW-1.39-notes (1.39.0-wmf.25; 2022-08-15).Aug 29 2022, 2:00 PM
gerritbot added a comment.Aug 30 2022, 8:53 AM

Change 615684 had a related patch set uploaded (by Tim Starling; author: Legoktm):

[mediawiki/core@master] rdbms: Move Database::factory() to DatabaseFactory service

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

gerritbot added a comment.Aug 31 2022, 11:45 PM

Change 822696 had a related patch set uploaded (by Krinkle; author: Aaron Schulz):

[mediawiki/core@master] rdbms: avoid DB_PRIMARY queries in getLagFromPtHeartbeat()

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

gerritbot added a comment.Sep 1 2022, 12:06 AM

Change 822696 merged by jenkins-bot:

[mediawiki/core@master] rdbms: avoid DB_PRIMARY queries in getLagFromPtHeartbeat()

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

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.28; 2022-09-05); removed MW-1.39-notes (1.39.0-wmf.27; 2022-08-29).Sep 1 2022, 1:00 AM
gerritbot added a comment.Sep 2 2022, 1:51 AM

Change 615684 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Move Database::factory() to DatabaseFactory service

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

gerritbot added a comment.Oct 12 2022, 4:25 PM

Change 824722 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Introduce DatabaseFlagsHolder and move some internal logic there

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

ReleaseTaggerBot added a project: MW-1.40-notes (1.40.0-wmf.6; 2022-10-17).Oct 12 2022, 5:00 PM
Krinkle added a subtask: T323663: Simplify Database::ping() and avoid calling it from pendingWriteQueryDuration().Nov 28 2022, 7:11 PM
Krinkle closed subtask T323663: Simplify Database::ping() and avoid calling it from pendingWriteQueryDuration() as Resolved.
gerritbot added a comment.Dec 15 2022, 12:54 AM

Change 824717 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Moving replication-related code to its own component

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

Ladsgroup updated the task description. (Show Details)Dec 15 2022, 8:43 AM
Ladsgroup closed this task as Resolved.Dec 15 2022, 9:11 AM
Ladsgroup claimed this task.

One major challenge for engineers while doing refactoring is knowing when to stop and consider it good enough (ref. "Kill it with fire" book). It's hard, there is always room for improvements and doing more. In this case of database class, we can probably move more code to SQLPlatform, probably set up a dedicated class for "Session" state and much more but:

  • It has already changed drastically, went from > 6000 lines to 3900 which 500 lines are just delegating to smaller classes.
    • This is more pronounced in subclasses. DatabaseMysqlBase went from 1600 lines to 800.
  • It's not longer the most complex class of mediawiki according to this metric: https://doc.wikimedia.org/mediawiki-core/master/phpmetrics/complexity.html
  • Its complexity score has been halved.
  • It's much more testable now.
  • I think its size is no longer rdbms' biggest problem. I can think of several other issues in rdbms. Notably undue complexity of LB/LBF, LoadMonitor's mess, removable complexity in Database class itself, reliance of running regex on SQL (e.g. Database::getTempTableWrites) and many more issues that are more important that Database class's size now.

So I'm closing this, we probably would open a new one in one or two years to revisit but in the mean time, let's fix more important issues.

Krinkle closed subtask T302953: Deprecate wasDeadlock/wasLockTimeout/wasConnectionLoss as public IDatabase methods as Resolved.Apr 12 2023, 12:51 AM
Maintenance_bot removed a project: Patch-For-Review.Apr 12 2023, 1:10 AM
Krinkle closed subtask T314094: Deprecate and remove IDatabase::lastQuery() as Resolved.Aug 11 2023, 4:29 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL