Page MenuHomePhabricator

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

Description

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

AC

  • 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

Related Gerrit Patches:
mediawiki/extensions/Wikibase : masterRemove try and catches that don't provide much value
mediawiki/extensions/Wikibase : wmf/1.35.0-wmf.5Do not catch DBError in ReplicaMasterAwareRecordIdsAcquirer.
mediawiki/extensions/Wikibase : masterDo not catch DBError in ReplicaMasterAwareRecordIdsAcquirer.

Event Timeline

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

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.

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

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

alaa_wmde updated the task description. (Show Details)

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?

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

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

alaa_wmde closed this task as Resolved.Nov 26 2019, 1:56 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

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