Skip to content
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

Warn user if the state is probably corrupted (one of previous migrations was interrupted) #6279

Closed
wants to merge 23 commits into from

Conversation

EdvardD
Copy link
Contributor

@EdvardD EdvardD commented Feb 11, 2022

Config changes:

  • Added a flag 'fail_if_migration_is_in_progress' (default to false) to fail node if previously migration was interrupted.

State changes:

  • Added a key 'migration_is_in_progress' to the column BlockMisc. Normally the flag doesn't exist or equals false. Equals true iff the last migration was interrupted in the middle. If the flag is true the error message will be shown on the node start. Depending on the flag 'fail_if_migration_is_in_progress' node start will fail.
  • Added a key 'store_is_probably_broken'. Normally the flag doesn't exist. Equals true iff previously the migration was done having another interrupted migration. The flag is permanent and is used to show an error message so the user is aware that the state can be corrupted.

nearcore/src/lib.rs Outdated Show resolved Hide resolved
nearcore/src/lib.rs Outdated Show resolved Hide resolved
nearcore/src/lib.rs Outdated Show resolved Hide resolved
nearcore/src/lib.rs Outdated Show resolved Hide resolved
nearcore/src/lib.rs Outdated Show resolved Hide resolved
@mm-near mm-near requested a review from nikurt February 11, 2022 12:52
@nikurt
Copy link
Contributor

nikurt commented Feb 11, 2022

I've just made #6282 which doesn't write anything to DB, but makes a persistent checkpoint to disk. This way we can not only detect if DB is probably corrupted, but can also recover without re-downloading a snapshot.

@abacabadabacaba
Copy link
Collaborator

Why don't use transactions to make migrations atomic?

@EdvardD
Copy link
Contributor Author

EdvardD commented Feb 14, 2022

Why don't use transactions to make migrations atomic?

It seems a single migration can be too big (the whole storage update) and putting it into a single transaction can lead to issues. That's my guess seeing the implementation.

Anyways @nikurt 's change uses checkpoints to guarantee migrations being transactional so I guess this change is not needed anymore.

@EdvardD EdvardD requested a review from mm-near February 14, 2022 16:16
@stale
Copy link

stale bot commented Feb 28, 2022

This PR has been automatically marked as stale because it has not had recent activity in the 2 weeks.
It will be closed in 3 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Feb 28, 2022
@mm-near mm-near removed their request for review March 1, 2022 10:16
@stale stale bot removed the S-stale label Mar 1, 2022
@mm-near
Copy link
Contributor

mm-near commented Mar 1, 2022

let's abandon this PR for now - in favor of @nikurt approach with checkpoints.

@bowenwang1996 bowenwang1996 deleted the warn_user_if_the_state_is_inconsistent branch March 1, 2022 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants