Do not catch DBError in new store db implementations
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	• alaa_wmde
	Nov 8 2019, 2:59 PM

Description

Context
We might be catching DBError in new store, which is extremely discouraged.

Find where we do that
- Only In ReplicaMasterAwareRecordIdsAcquirer#insertNonExistingRecordsIntoMaster we are catching a DBError without throwing it again,
- We also catch it, but throw it again in two places: DatabaseItemTermStore#storeTerms and DatabasePropertyTermStore#storeTerms
Make sure to either not catch DBError or to rethrow it again in those places

Details

Subject	Repo	Branch	Lines +/-
Remove try and catches that don't provide much value	mediawiki/extensions/Wikibase	master	+84 -166
Do not catch DBError in ReplicaMasterAwareRecordIdsAcquirer.	mediawiki/extensions/Wikibase	wmf/1.35.0-wmf.5	+1 -14
Do not catch DBError in ReplicaMasterAwareRecordIdsAcquirer.	mediawiki/extensions/Wikibase	master	+1 -14

Customize query in gerrit

Related Objects
Search...

Status	Subtype	Assigned	Task
Resolved		Addshore	T208425 [EPIC] Kill the wb_terms table
Duplicate	PRODUCTION ERROR	None	T237723 PHP Warning: [data-update-failed]: Fail-safe exception. Number of ids to be restored is not equal to the number of records that are about to be inserted into master.….) [Called from Wikibase/repo/includes/Content/DataUpdateAdapter.php]
Resolved		• alaa_wmde	T237731 Do not catch DBError in new store db implementations

Event Timeline

• alaa_wmde created this task.Nov 8 2019, 2:59 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 8 2019, 2:59 PM

Change 549864 had a related patch set uploaded (by Alaa Sarhan; owner: Alaa Sarhan):
[mediawiki/extensions/Wikibase@master] Do not catch DBError in ReplicaMasterAwareRecordIdsAcquirer.

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

gerritbot added a project: Patch-For-Review.Nov 8 2019, 3:03 PM

• alaa_wmde added a parent task: T237723: PHP Warning: [data-update-failed]: Fail-safe exception. Number of ids to be restored is not equal to the number of records that are about to be inserted into master.….) [Called from Wikibase/repo/includes/Content/DataUpdateAdapter.php].Nov 8 2019, 3:05 PM

• alaa_wmde added projects: Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)), Wikidata.

• alaa_wmde moved this task from To Do (prioritised from top to bottom) to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.

Maintenance_bot moved this task from incoming to in progress on the Wikidata board.Nov 8 2019, 3:15 PM

We catch it in DatabasePropertyTermStore::cleanTermsIfUnused() and same for items. We should stop doing it.

Ladsgroup moved this task from Peer Review to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Nov 9 2019, 5:05 PM

Change 549864 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Do not catch DBError in ReplicaMasterAwareRecordIdsAcquirer.

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

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.8; 2019-11-26).Nov 9 2019, 6:00 PM

Maintenance_bot removed a project: Patch-For-Review.Nov 9 2019, 6:10 PM

https://tools.wmflabs.org/versions/
We have wmf.5 in production now. This one is tagged for wmf.8. is that going to be the branch for this week's train?

In T237731#5652353, @alaa_wmde wrote:

https://tools.wmflabs.org/versions/
We have wmf.5 in production now. This one is tagged for wmf.8. is that going to be the branch for this week's train?

This week is train freeze due to tech conf. I can back port this patch though.

Please don't move it done, I moved it back to doing because there is lots of places that we still catch db error in the new term store. I exampled one above.

Okay, I tested and in all of cases, we rethrow exception which I don't we should catch them in the first place, the stack trace will show up anyway, but that's not a big deal.

Change 550441 had a related patch set uploaded (by Ladsgroup; owner: Alaa Sarhan):
[mediawiki/extensions/Wikibase@wmf/1.35.0-wmf.5] Do not catch DBError in ReplicaMasterAwareRecordIdsAcquirer.

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

gerritbot added a project: Patch-For-Review.Nov 12 2019, 10:40 AM

Change 550441 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@wmf/1.35.0-wmf.5] Do not catch DBError in ReplicaMasterAwareRecordIdsAcquirer.

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

Maintenance_bot removed a project: Patch-For-Review.Nov 12 2019, 12:10 PM

ReleaseTaggerBot edited projects, added MW-1.35-notes (1.35.0-wmf.5; 2019-11-05); removed MW-1.35-notes (1.35.0-wmf.8; 2019-11-26).Nov 12 2019, 1:00 PM

Let's remove catching DBError from DatabaseItemTermStore#storeTerms and DatabasePropertyTermStore#storeTerms separately, as they don't really provide that much valuable info in the error on top of the DBError anyway

Change 553115 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Remove try and catches that don't provide much value

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

gerritbot added a project: Patch-For-Review.Nov 26 2019, 2:11 PM

Change 553115 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove try and catches that don't provide much value

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

ReleaseTaggerBot edited projects, added MW-1.35-notes (1.35.0-wmf.10; 2019-12-10); removed MW-1.35-notes (1.35.0-wmf.5; 2019-11-05).Nov 26 2019, 9:00 PM

Maintenance_bot removed a project: Patch-For-Review.Nov 26 2019, 9:10 PM