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

Make migration api more friendly #2420

Merged
merged 2 commits into from
Oct 1, 2016
Merged

Make migration api more friendly #2420

merged 2 commits into from
Oct 1, 2016

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Sep 30, 2016

once it is passed with arc reference, original database can now be used in more complex queries

@NikVolf NikVolf added the A0-pleasereview 🤓 Pull request needs code review. label Sep 30, 2016
Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

one problem with passing an Arc is that it allows migrations to Clone it or create weak pointers, which might prevent the destructor (which closes the rocksdb handle) from ever being run before the directory is deleted.

I think this is fine as a patch but it doesn't fully address the problem that the migration manager isn't suited to our current database format.

@@ -257,7 +258,7 @@ impl Migration for OverlayRecentV7 {
try!(batch.insert(key, value.into_vec(), dest));
}

try!(self.walk_journal(source));
try!(self.walk_journal(source.clone()));
Copy link
Contributor

Choose a reason for hiding this comment

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

don't see why a clone is necessary here. works just fine with a reference.

@rphmeier rphmeier added the M4-core ⛓ Core client code / Rust. label Sep 30, 2016
@rphmeier
Copy link
Contributor

This PR should also allow safer in-place upgrades (by running complex migration logic once based on the given column rather than running them after every migration) and prevent issues like #2411.

@NikVolf
Copy link
Contributor Author

NikVolf commented Sep 30, 2016

@rphmeier yeah, this was always the plan

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.711% when pulling 5fe3b47 on migration-api-friendly into 1d3e242 on master.

@NikVolf NikVolf mentioned this pull request Oct 1, 2016
@arkpar arkpar added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 1, 2016
@arkpar arkpar merged commit 1029f84 into master Oct 1, 2016
jacogr added a commit that referenced this pull request Oct 1, 2016
* js:
  signaturereg registered, remove hardcoding
  fixes for non-null returns
  update ABIs to latest deployed versions
  update Morden registry address (#2417)
  using arc (#2420)
  asterisk space
  removed redundant memcopy
  Update gitlab-ci
  Fixing logs-receipt matching (#2403)
  fix broken beta compilation
  Fixing transaction queue (#2392)
  separate mod for tests
  bloom filter crate
@arkpar arkpar deleted the migration-api-friendly branch October 3, 2016 15:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants