Page MenuHomePhabricator

[keyserver] only wrap migration in transaction if specified in migrationType
ClosedPublic

Authored by will on Aug 30 2024, 8:29 AM.
Tags
None
Referenced Files
F3570629: D13212.id43952.diff
Sat, Dec 28, 5:51 AM
F3570618: D13212.id43958.diff
Sat, Dec 28, 5:51 AM
Unknown Object (File)
Wed, Dec 18, 7:15 AM
Unknown Object (File)
Wed, Dec 18, 7:15 AM
Unknown Object (File)
Wed, Dec 18, 7:14 AM
Unknown Object (File)
Mon, Dec 2, 12:26 AM
Unknown Object (File)
Nov 29 2024, 2:57 AM
Unknown Object (File)
Nov 28 2024, 10:14 PM
Subscribers

Details

Summary

If the migration type is wrap_in_transaction_and_block_requests, we wrap the migration in a transaction. Only if it was wrapped in transaction do we run rollback if the query failed.

Depends on D13209

Test Plan

I console logged and made sure that transaction and rollback queries were only being run if the migration in migrations specified wrapping in a transaction.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 30 2024, 8:40 AM
Harbormaster failed remote builds in B31372: Diff 43816!
will requested review of this revision.Aug 30 2024, 9:00 AM
ashoat added inline comments.
keyserver/src/database/migrations.js
51–52 ↗(On Diff #43816)

Let's avoid copy-pasting these two lines. Can't we just individually wrap startTransaction and commitTransaction in the conditional?

This revision is now accepted and ready to land.Sep 4 2024, 11:20 PM
keyserver/src/database/migrations.js
51–52 ↗(On Diff #43816)

That makes sense! I originally had them both individually wrapped but thought this looked cleaner. I'll go by the maxim of reducing copy-paste of non-conditionals from now on