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

update_migration_17_18 #4055

Merged
merged 6 commits into from
Mar 14, 2021
Merged

update_migration_17_18 #4055

merged 6 commits into from
Mar 14, 2021

Conversation

Kouprin
Copy link
Member

@Kouprin Kouprin commented Mar 6, 2021

Fixes #4054.

Tested on gc_sync_after_sync.py and state_sync_late.py tests.

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kouprin it appears to me that this would still loop over all key values in ColBlockHeader. Did I miss something?

@Kouprin
Copy link
Member Author

Kouprin commented Mar 8, 2021

@bowenwang1996 yes. I do think reading 20 mln Headers shouldn't take up for several hours. Please check my comment #4054 (comment).

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me. As to the approach, I let the chain team decide.

@bowenwang1996
Copy link
Collaborator

@Kouprin did you have a chance to test it with mainnet data?

@Kouprin
Copy link
Member Author

Kouprin commented Mar 9, 2021

@bowenwang1996

      ~/n/nearcore     update_migration_17_18  time target/release/neard run
Mar 09 17:09:55.543  INFO near: Version: 1.2.0, Build: 371263b4, Latest Protocol: 42
Mar 09 17:09:55.548  INFO near: Opening store database at "/Users/kpr/.near/data"
Mar 09 17:09:55.603  INFO near: Migrate DB from version 16 to 17
Mar 09 17:09:55.972  INFO near: Migrate DB from version 17 to 18
Mar 09 17:23:14.541  INFO stats: Server listening at ed25519:7vnK4qivmu96g1UALT4y9thZw75ye3cNmw9EcP2qxEb9@0.0.0.0:24567
^Ctarget/release/neard run  630.29s user 178.98s system 100% cpu 13:28.98 total

It takes 13 minutes on my laptop. Should we proceed with this solution then?

@frol
Copy link
Collaborator

frol commented Mar 9, 2021

Off-topic: @bowenwang1996 @chefsale can we add CI jobs that try booting up a node from testnet and mainnet backups (RPC and archival) with some timeout? We had quite a few issues with config.json parsing, sync, migrations, etc. Well, I think what I just described is designed to be the canary build. I wonder if we can automate canary builds to be manually triggered for PRs.

@bowenwang1996
Copy link
Collaborator

@frol we definitely can. The issue is that then we have to set a pretty small timeout (not more than 10 minutes for example) to avoid blocking CI.

@chefsale
Copy link
Contributor

chefsale commented Mar 12, 2021

@frol @bowenwang1996 we currently run a canary on testnet which runs master, if we added the CI you would have a small canary time as mentioned.

It is a daily canary and could be easily changed to run on every commit instead of a scheduled time. Canary pretty much catches all these issues right now, but on a daily matter and we manually revert the change if this causes an issue.

One of the ideas I had was to create a job which would automatically revert from master if there was a commit which broke the node.

@near-bulldozer near-bulldozer bot merged commit 7f989ba into master Mar 14, 2021
@near-bulldozer near-bulldozer bot deleted the update_migration_17_18 branch March 14, 2021 13:57
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.

DB migration 17 -> 18 is too disruptive
4 participants