Page MenuHomePhabricator

Hard deprecate Revision use in WikiPage::doEditUpdates
Closed, ResolvedPublic

Description

Soft deprecated in 1.32

Only remaining uses are in core

1.35: Converted to accept RevisionRecord, using Revision hard deprecated.

Event Timeline

Restricted Application added a project: Wikidata. · View Herald TranscriptApr 6 2020, 10:23 PM
DannyS712 triaged this task as Medium priority.Apr 6 2020, 10:23 PM
DannyS712 moved this task from Unsorted to In progress on the User-DannyS712 board.

I took another look - part of the issue is that I can't tell what the code is meant to be doing. @Lucas_Werkmeister_WMDE @Addshore could one of you shed some light of the this?

This SqlStore::rebuild() method seems to date back to 2012, with relatively little substantial changes since then. Apart from one test, it’s only called in DatabaseSchemaUpdater::doSchemaUpdate(), when updating from older database schemas. My guess is that the method is really meant to “rebuild the entity store”; maybe we used to register deferred edit updates to migrate stored revisions to a new serialization schema, and calling doEditUpdates() in this rebuild() method was meant to do the same thing for all pages, even if they hadn’t received an edit lately (and therefore didn’t get a deferred edit update through the normal route).

It’s worth noting that, as far as I’m aware, Wikibase hasn’t actually been updating old revisions for a long time now; instead, Wikibase can parse old serialization formats for the purpose of reading old revisions (but will only write new revisions with the latest serialization format). Also, SqlStore::rebuild() will only actually rebuild the first 1000 pages (“TODO: continuation”), so it would only do the right thing when upgrading very old Wikibases that are also very small (because, no matter whether the rebuild() touched all pages or only some of them, the rest of DatabaseSchemaUpdater::doSchemaUpdate() will have run already, and so the next database updates will no longer end up calling rebuild()).

@Ladsgroup might have something to add because he also looked at related code recently (that same Gerrit change also caused T249565, I believe).

This SqlStore::rebuild() method seems to date back to 2012, with relatively little substantial changes since then. Apart from one test, it’s only called in DatabaseSchemaUpdater::doSchemaUpdate(), when updating from older database schemas. My guess is that the method is really meant to “rebuild the entity store”; maybe we used to register deferred edit updates to migrate stored revisions to a new serialization schema, and calling doEditUpdates() in this rebuild() method was meant to do the same thing for all pages, even if they hadn’t received an edit lately (and therefore didn’t get a deferred edit update through the normal route).

It’s worth noting that, as far as I’m aware, Wikibase hasn’t actually been updating old revisions for a long time now; instead, Wikibase can parse old serialization formats for the purpose of reading old revisions (but will only write new revisions with the latest serialization format). Also, SqlStore::rebuild() will only actually rebuild the first 1000 pages (“TODO: continuation”), so it would only do the right thing when upgrading very old Wikibases that are also very small (because, no matter whether the rebuild() touched all pages or only some of them, the rest of DatabaseSchemaUpdater::doSchemaUpdate() will have run already, and so the next database updates will no longer end up calling rebuild()).

@Ladsgroup might have something to add because he also looked at related code recently (that same Gerrit change also caused T249565, I believe).

So can the method be removed entirely?

Likely, but I don’t know if we want to support upgrading old wikibases or not (if yes, we might need a replacement for that method). @Addshore, do you know?

Likely, but I don’t know if we want to support upgrading old wikibases or not (if yes, we might need a replacement for that method). @Addshore, do you know?

I'm a little confused now, you said (and AFAIK) that "Wikibase can parse old serialization formats for the purpose of reading old revisions (but will only write new revisions with the latest serialization format). " which means it'll just slowly rebuilds the new store, so removing it wouldn't break upgrading old wikibases. Am I missing something obvious?

Okay, Wikibase can parse some old serialization formats. But I have no idea if this is also true for whatever serialization format we used back in 2012, because it also seems plausible to me that we would have relied on the rebuild script until then, and only started to support old serialization formats from after that point.

(Also, the store is never rebuilt during normal operation, unless we mean different things… I don’t think Wikibase ever updates those old revisions.)

Ladsgroup added a comment.EditedJun 3 2020, 8:16 PM

Okay, Wikibase can parse some old serialization formats. But I have no idea if this is also true for whatever serialization format we used back in 2012, because it also seems plausible to me that we would have relied on the rebuild script until then, and only started to support old serialization formats from after that point.

hmm, AFAIK we only had two serializations, one old way and one we are currently using. Some other people might know better

I believe this is true but adding @daniel to confirm.

Change 604188 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Wikibase@master] Remove SqlStore::rebuild

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

Change 604188 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove SqlStore::rebuild

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

DannyS712 updated the task description. (Show Details)Jun 10 2020, 4:48 PM

Change 605684 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add internal WikiPage::doPostEditUpdates

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

Since there is no clear way to replace uses, I added an internal stop-gap replacement so that this doesn't hold up deprecating the Revision class.

Change 605684 merged by jenkins-bot:
[mediawiki/core@master] WikiPage::doEditUpdates - accept a RevisionRecord object

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

DannyS712 renamed this task from Hard deprecate WikiPage::doEditUpdates to Hard deprecate Revision use in WikiPage::doEditUpdates.Jun 18 2020, 10:12 AM
DannyS712 closed this task as Resolved.
DannyS712 claimed this task.
DannyS712 updated the task description. (Show Details)

A separate task should be created to hard deprecate the method and then remove it entirely; this task originally proposed hard deprecating due to the use of Revision, but instead passing a Revision was just hard deprecated by itself.