Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Run inplace upgrades after version update #2411

Merged
merged 2 commits into from
Oct 7, 2016
Merged

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Sep 29, 2016

In highly unlikely scenario when the bloom update fails it will at least try again on the next run

In highly unlikely scenario when the bloom update fails it will at least try again on the next run
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. B0-patch A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 29, 2016
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 29, 2016
@rphmeier rphmeier added the M4-core ⛓ Core client code / Rust. label Sep 30, 2016
@rphmeier
Copy link
Contributor

rphmeier commented Sep 30, 2016

but following migrations which expect a bloom column to be populated won't find it if in any migration situation other than 10 -> 11. What about 9 -> 11? And also, it looks like the inplace migrations don't check version. Do they each have to include a check for whether they've already been run? That information should be available from the current version.

@NikVolf
Copy link
Contributor Author

NikVolf commented Sep 30, 2016

@rphmeier
this is just workaround for one particular hypothetical problem (that is unlikely ever happen), pr does not anwer all your concerns about such in-place migrations in any way :)

@rphmeier
Copy link
Contributor

I think this can be superseded by https://github.com/ethcore/parity/pull/2420 if the inplace migration is moved to the ToV10 migration there.

@rphmeier rphmeier added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 30, 2016
@NikVolf
Copy link
Contributor Author

NikVolf commented Sep 30, 2016

@rphmeier that is my plan for master, i don't want to do it in beta

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Sep 30, 2016
@gavofyork
Copy link
Contributor

@rphmeier please mark looksgood/mustntgrumble (to allow merge) or grumble (to escalate further).

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 5, 2016
@rphmeier
Copy link
Contributor

rphmeier commented Oct 5, 2016

I'll mark it Looksgood although I don't really agree with the strategy as a whole because

  • Beta will soon become stable
  • stable will probably not see any future DB upgrades backported and won't be hit by the issues I've pinpointed

@arkpar arkpar merged commit 9b39842 into beta Oct 7, 2016
@gavofyork gavofyork deleted the migration-beta-patch branch November 3, 2016 11:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants