-
Notifications
You must be signed in to change notification settings - Fork 213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
widen 'isMigrationError' to catch constraint errors #1261
Conversation
On start-up, we run persistent migrations and in case of migration issues, we execute some particular handling (e.g. the pool db resets itself). Yet, we failed at catching migration issues coming from constraints introduced later and that can't be satisfied on already existing data. This commit fixes it. Before ====== [cardano-wallet.network:Info:22] Jörmungandr is ready. cardano-wallet-jormungandr: SQLite3 returned ErrorConstraint while attempting to perform step: NOT NULL constraint failed: pool_owner_backup.pool_owner_index (exiting with an error code) After ===== [cardano-wallet.network:Info:22] Jörmungandr is ready. [cardano-wallet.stake-pool-db:Error:22] Failed to migrate the database: : NOT NULL constraint failed: pool_owner_backup.pool_owner_index [cardano-wallet.stake-pool-db:Notice:22] Non backward compatible database found. Removing old database and re-creating it from scratch. Ignore the previous error. [cardano-wallet.stake-pool-db:Notice:22] 1 migrations were applied to the database. (... and continuing as normal)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this going from v2019-12-13
(f3576be) to the current master
(10b5512), and it does appear to solve the problem of recreating the stake pool DB:
[cardano-wallet.stake-pool-db:Error:22] [2020-01-14 02:52:08.72 UTC] Failed to migrate the database: : NOT NULL constraint failed: pool_owner_backup.pool_owner_index
[cardano-wallet.stake-pool-db:Notice:22] [2020-01-14 02:52:08.72 UTC] Non backward compatible database found. Removing old database and re-creating it from scratch. Ignore the previous error.
I only have a small suggestion regarding naming of the function isMigrationError
. (See comments.)
lib/core/src/Cardano/DB/Sqlite.hs
Outdated
mark = "Database migration: manual intervention required." | ||
class Exception e => IsMigrationError e where | ||
-- | Exception predicate for migration errors. | ||
isMigrationError :: e -> Maybe MigrationError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the function name isMigrationError
is a bit misleading, as the prefix is
strongly implies a Boolean result, when what we actually want is to match and extract an error of a particular type (if it exists).
Suggestion: rename this to something like catchMigrationError
or matchMigrationError
.
bors r+ |
1261: widen 'isMigrationError' to catch constraint errors r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #1251 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> On start-up, we run persistent migrations and in case of migration issues, we execute some particular handling (e.g. the pool db resets itself). Yet, we failed at catching migration issues coming from constraints introduced later and that can't be satisfied on already existing data. This commit fixes it. Before ====== ``` [cardano-wallet.network:Info:22] Jörmungandr is ready. cardano-wallet-jormungandr: SQLite3 returned ErrorConstraint while attempting to perform step: NOT NULL constraint failed: pool_owner_backup.pool_owner_index (exiting with an error code) ``` After ===== ``` [cardano-wallet.network:Info:22] Jörmungandr is ready. [cardano-wallet.stake-pool-db:Error:22] Failed to migrate the database: : NOT NULL constraint failed: pool_owner_backup.pool_owner_index [cardano-wallet.stake-pool-db:Notice:22] Non backward compatible database found. Removing old database and re-creating it from scratch. Ignore the previous error. [cardano-wallet.stake-pool-db:Notice:22] 1 migrations were applied to the database. (... and continuing as normal) ``` # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Build succeeded |
Issue Number
#1251
Overview
widen 'isMigrationError' to catch constraint errors
On start-up, we run persistent migrations and in case of migration issues, we
execute some particular handling (e.g. the pool db resets itself). Yet, we
failed at catching migration issues coming from constraints introduced later
and that can't be satisfied on already existing data. This commit fixes it.
Before
After
25fa318
rename 'IsMigration...' to 'MatchMigration...'
9d61437
flatten result after tryJust to a single 'Either'
Comments